| Message ID | 20201124121630.20453.20417.stgit@dceara.remote.csb |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ovsdb-idl: Fix IDL use-after-free and memory leak issues. | expand |
On Tue, Nov 24, 2020 at 4:16 AM Dumitru Ceara <dceara@redhat.com> wrote: > > 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. Hi Dumitru, does this change suggest that there are situations that a deleted row is tracked but there is no tracked_old_datum? Could you give an example of how it could happen? If a client didn't see a row "inserted" then how could the server send a notification about the "deletion"? If a row is never seen by a client IDL then I think it shouldn't be included in the update notification. > > Reported-by: Ilya Maximets <i.maximets@ovn.org> > Fixes: 72aeb243a52a ("ovsdb-idl: Tracking - preserve data for deleted rows.") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > lib/ovsdb-idl.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index bbc22e4..b282ce4 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -1969,16 +1969,18 @@ ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db) > > ovs_list_remove(&row->track_node); > ovs_list_init(&row->track_node); > - if (ovsdb_idl_row_is_orphan(row) && row->tracked_old_datum) { > + if (ovsdb_idl_row_is_orphan(row)) { > ovsdb_idl_row_unparse(row); > - const struct ovsdb_idl_table_class *class = > - row->table->class_; > - for (size_t c = 0; c < class->n_columns; c++) { > - ovsdb_datum_destroy(&row->tracked_old_datum[c], > - &class->columns[c].type); > + if (row->tracked_old_datum) { > + const struct ovsdb_idl_table_class *class = > + row->table->class_; > + for (size_t c = 0; c < class->n_columns; c++) { > + ovsdb_datum_destroy(&row->tracked_old_datum[c], > + &class->columns[c].type); > + } > + free(row->tracked_old_datum); > + row->tracked_old_datum = NULL; > } > - free(row->tracked_old_datum); > - row->tracked_old_datum = NULL; > free(row); > } > } > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 11/30/20 5:52 AM, Han Zhou wrote: > > > On Tue, Nov 24, 2020 at 4:16 AM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> wrote: >> >> 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. > > Hi Dumitru, does this change suggest that there are situations that a > deleted row is tracked but there is no tracked_old_datum? Could you give > an example of how it could happen? If a client didn't see a row > "inserted" then how could the server send a notification about the > "deletion"? If a row is never seen by a client IDL then I think it > shouldn't be included in the update notification. > Hi Han, Ilya recently added a test case that exercises this exact scenario: https://github.com/openvswitch/ovs/blob/fa57e9e45257f32b80c135c18ae821ac3c43a738/tests/ovsdb-idl.at#L1178 The "simple6" record in the test has weak references to "simple" records "weak_row0", "weak_row1", "weak_row2". But the monitor condition for table "simple" is "s==row1_s". That means that the IDL will create "pure" orphan records for "weak_row0" and "weak_row1". When the "simple6" record is deleted, the two orphan rows above should also be deleted. For that they're added to the track_list. Without the fix, they'll just be removed from the list without ever being freed. Regards, Dumitru
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index bbc22e4..b282ce4 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -1969,16 +1969,18 @@ ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db) ovs_list_remove(&row->track_node); ovs_list_init(&row->track_node); - if (ovsdb_idl_row_is_orphan(row) && row->tracked_old_datum) { + if (ovsdb_idl_row_is_orphan(row)) { ovsdb_idl_row_unparse(row); - const struct ovsdb_idl_table_class *class = - row->table->class_; - for (size_t c = 0; c < class->n_columns; c++) { - ovsdb_datum_destroy(&row->tracked_old_datum[c], - &class->columns[c].type); + if (row->tracked_old_datum) { + const struct ovsdb_idl_table_class *class = + row->table->class_; + for (size_t c = 0; c < class->n_columns; c++) { + ovsdb_datum_destroy(&row->tracked_old_datum[c], + &class->columns[c].type); + } + free(row->tracked_old_datum); + row->tracked_old_datum = NULL; } - free(row->tracked_old_datum); - row->tracked_old_datum = NULL; free(row); } }
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@ovn.org> Fixes: 72aeb243a52a ("ovsdb-idl: Tracking - preserve data for deleted rows.") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- lib/ovsdb-idl.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)