| Message ID | 20201124121615.20453.10922.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: > > 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. 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 don't currently use the deleted old_datum values but they The old_datum which is set to tracked_old_datum is in fact used when the client accesses the data of the tracked deleted row. At least it was used in ovn-controller. > might decide to do that in the future 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. > > Fixes: 72aeb243a52a ("ovsdb-idl: Tracking - preserve data for deleted rows.") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > lib/ovsdb-idl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index 6334061..bbc22e4 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -3219,7 +3219,7 @@ ovsdb_idl_row_clear_old(struct ovsdb_idl_row *row) > { > ovs_assert(row->old_datum == row->new_datum); > if (!ovsdb_idl_row_is_orphan(row)) { > - if (ovsdb_idl_track_is_set(row->table)) { > + if (ovsdb_idl_track_is_set(row->table) && !row->tracked_old_datum) { > row->tracked_old_datum = row->old_datum; > } else { > const struct ovsdb_idl_table_class *class = row->table->class_; > Thanks for the patch. The change looks correct (at least not harmful), but I'm not sure if I understand the scenario completely. In step 2, how could someone insert a row that has already been deleted? Does the client specify the row UUID in this case? Would it be better to add a test case which could trigger the problem? Thanks, Han
On 11/30/20 5:22 AM, Han Zhou wrote: > > > On Tue, Nov 24, 2020 at 4:16 AM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> wrote: >> >> 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. 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 don't currently use the deleted old_datum values but they > > The old_datum which is set to tracked_old_datum is in fact used when the > client accesses the data of the tracked deleted row. At least it was > used in ovn-controller. > You're right, I'll update the commit log. >> might decide to do that in the future 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. >> >> Fixes: 72aeb243a52a ("ovsdb-idl: Tracking - preserve data for deleted > rows.") >> Signed-off-by: Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> >> --- >> lib/ovsdb-idl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c >> index 6334061..bbc22e4 100644 >> --- a/lib/ovsdb-idl.c >> +++ b/lib/ovsdb-idl.c >> @@ -3219,7 +3219,7 @@ ovsdb_idl_row_clear_old(struct ovsdb_idl_row *row) >> { >> ovs_assert(row->old_datum == row->new_datum); >> if (!ovsdb_idl_row_is_orphan(row)) { >> - if (ovsdb_idl_track_is_set(row->table)) { >> + if (ovsdb_idl_track_is_set(row->table) && > !row->tracked_old_datum) { >> row->tracked_old_datum = row->old_datum; >> } else { >> const struct ovsdb_idl_table_class *class = > row->table->class_; >> > > Thanks for the patch. The change looks correct (at least not harmful), > but I'm not sure if I understand the scenario completely. In step 2, how > could someone insert a row that has already been deleted? Does the > client specify the row UUID in this case? Would it be better to add a > test case which could trigger the problem? > This is from the IDL's perspective. A "delete"/"insert" update message received from ovsdb-server doesn't necessarily mean that row R1 was actually deleted/inserted in the DB. It means the row has to be deleted/inserted from the client's view of the DB. One such case is when monitor conditions change. As a matter of fact I managed to replicate it now without OVN so I'll be adding a test case for it in the new revision. > Thanks, > Han Thanks, Dumitru
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index 6334061..bbc22e4 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -3219,7 +3219,7 @@ ovsdb_idl_row_clear_old(struct ovsdb_idl_row *row) { ovs_assert(row->old_datum == row->new_datum); if (!ovsdb_idl_row_is_orphan(row)) { - if (ovsdb_idl_track_is_set(row->table)) { + if (ovsdb_idl_track_is_set(row->table) && !row->tracked_old_datum) { row->tracked_old_datum = row->old_datum; } else { const struct ovsdb_idl_table_class *class = row->table->class_;
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. 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 don't currently use the deleted old_datum values but they might decide to do that in the future 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. Fixes: 72aeb243a52a ("ovsdb-idl: Tracking - preserve data for deleted rows.") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- lib/ovsdb-idl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)