[ovs-git] [openvswitch/ovs] 08c8a2: ovsdb-idl: Fix memleak when reinserting tracked or...

[Dumitru Ceara]

  Branch: refs/heads/branch-2.13
  Home:   https://github.com/openvswitch/ovs
  Commit: 08c8a29bf4de0650584eba70827b414b4ac71a78
      https://github.com/openvswitch/ovs/commit/08c8a29bf4de0650584eba70827b414b4ac71a78
  Author: Dumitru Ceara <dceara at redhat.com>
  Date:   2020-12-03 (Thu, 03 Dec 2020)

  Changed paths:
    M lib/ovsdb-idl.c
    M tests/ovsdb-idl.at

  Log Message:
  -----------
  ovsdb-idl: Fix memleak when reinserting tracked orphan rows.

Considering the following updates processed by an IDL client:
1. Delete row R1 from table A while R1 is also referenced by row R2 from
   table B:
   - because row R2 still refers to row R1, this will create an orphan
     R1 but also sets row->tracked_old_datum to report to the IDL client
     that the row has been deleted.
2. Insert row R1 to table A.
   - because orphan R1 already existed in the IDL, it will be reused.
   - R1 still has row->tracked_old_datum set (and may also be on the
     table->track_list).
3. Delete row R2 from table B and row R1 from table A.
   - row->tracked_old_datum is set again but the previous
     tracked_old_datum was never freed.

IDL clients use the deleted old_datum values so when multiple delete
operations are received for a row, always track the first one as that
will match the contents of the row the IDL client knew about.

Running the newly added test case with valgrind, without the fix,
produces the following report:

==23113== 327 (240 direct, 87 indirect) bytes in 1 blocks are definitely lost in loss record 43 of 43
==23113==    at 0x4C29F73: malloc (vg_replace_malloc.c:309)
==23113==    by 0x476761: xmalloc (util.c:138)
==23113==    by 0x45D8B3: ovsdb_idl_insert_row (ovsdb-idl.c:3431)
==23113==    by 0x45B7F9: ovsdb_idl_process_update2 (ovsdb-idl.c:2670)
==23113==    by 0x45AFCF: ovsdb_idl_db_parse_update__ (ovsdb-idl.c:2479)
==23113==    by 0x45B262: ovsdb_idl_db_parse_update (ovsdb-idl.c:2542)
==23113==    by 0x45ABBE: ovsdb_idl_db_parse_update_rpc (ovsdb-idl.c:2358)
==23113==    by 0x4576DD: ovsdb_idl_process_msg (ovsdb-idl.c:865)
==23113==    by 0x457973: ovsdb_idl_run (ovsdb-idl.c:944)
==23113==    by 0x40B7B9: do_idl (test-ovsdb.c:2523)
==23113==    by 0x44425D: ovs_cmdl_run_command__ (command-line.c:247)
==23113==    by 0x44430E: ovs_cmdl_run_command (command-line.c:278)
==23113==    by 0x404BA6: main (test-ovsdb.c:76)

Fixes: 72aeb243a52a ("ovsdb-idl: Tracking - preserve data for deleted rows.")
Signed-off-by: Dumitru Ceara <dceara at redhat.com>
Acked-by: Han Zhou <hzhou at ovn.org>
Signed-off-by: Ilya Maximets <i.maximets at ovn.org>


  Commit: 60607ba779b839861d1832eb5b5147c54fadb178
      https://github.com/openvswitch/ovs/commit/60607ba779b839861d1832eb5b5147c54fadb178
  Author: Dumitru Ceara <dceara at redhat.com>
  Date:   2020-12-03 (Thu, 03 Dec 2020)

  Changed paths:
    M lib/ovsdb-idl.c

  Log Message:
  -----------
  ovsdb-idl: Fix memleak when deleting orphan rows.

Pure IDL orphan rows, i.e., for which no "insert" operation was seen,
which are part of tables with change tracking enabled should also be
freed when the table track_list is flushed.

Reported-by: Ilya Maximets <i.maximets at ovn.org>
Fixes: 72aeb243a52a ("ovsdb-idl: Tracking - preserve data for deleted rows.")
Signed-off-by: Dumitru Ceara <dceara at redhat.com>
Acked-by: Han Zhou <hzhou at ovn.org>
Signed-off-by: Ilya Maximets <i.maximets at ovn.org>


  Commit: ac5c067937190b930c4d75d47df739e98df720aa
      https://github.com/openvswitch/ovs/commit/ac5c067937190b930c4d75d47df739e98df720aa
  Author: Dumitru Ceara <dceara at redhat.com>
  Date:   2020-12-03 (Thu, 03 Dec 2020)

  Changed paths:
    M lib/ovsdb-idl.c

  Log Message:
  -----------
  ovsdb-idl: Fix use-after-free when deleting orphaned rows.

It's possible that the IDL client processes multiple jsonrpc updates
in a single ovsdb_idl_run().

Considering the following updates processed in a single IDL run:
1. Update row R1 from table A while R1 is also referenced by row R2 from
   table B:
   - this adds R1 to table A's track_list.
2. Delete row R1 from table A while R1 is also referenced by row R2 from
   table B:
   - because row R2 still refers to row R1, this will create an orphan
     R1.
   - at this point R1 is still in table A's hmap.

When the IDL client calls ovsdb_idl_track_clear() after it has finished
processing the tracked changes, row R1 gets freed leaving a dangling
pointer in table A's hmap.

To fix this we don't free rows in ovsdb_idl_track_clear() if they are
orphan and still referenced by other rows, i.e., the row's 'dst_arcs'
list is not empty.  Later, when all arc sources (e.g., R2) are
deleted, the orphan R1 will be cleaned up as well.

The only exception is when the whole contents of the IDL are flushed,
in ovsdb_idl_db_clear(), in which case it's safe to free all rows.

Reported-by: Ilya Maximets <i.maximets at ovn.org>
Fixes: 932104f483ef ("ovsdb-idl: Add support for change tracking.")
Signed-off-by: Dumitru Ceara <dceara at redhat.com>
Acked-by: Han Zhou <hzhou at ovn.org>
Signed-off-by: Ilya Maximets <i.maximets at ovn.org>


Compare: https://github.com/openvswitch/ovs/compare/d853716dcb6f...ac5c06793719