On 11/30/20 8:53 AM, Han Zhou wrote: > > > On Tue, Nov 24, 2020 at 4:16 AM Dumitru Ceara <[email protected] > <mailto:[email protected]>> 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. 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. > > Hi Dumitru, > > For my understanding, deleting R1 should result in an update of R2 if > the reference is weak, or failure of the transaction if the reference is > strong. > If it is a weak reference, then the single ovsdb_idl_run() should handle > both R1 deletion and the R2 updates which removes the reference to R1, > so row R1 should be removed from A's hmap when R2 update is handled. > > Could you explain how the problem of the dangling pointer happens? >
The "delete" R1 update the IDL client receives doesn't necessarily mean that the row was deleted from the DB. It can also happen as a result of the IDL client changing the monitor condition to not include R1's UUID in the condition for table A. In that case, there will be no update for R2. So R1 remains in the hmap as an orphan row because it's still referenced by R2 but it's also on the track list (due to update #1) and marked for deletion, tracked_old_datum != NULL (due to update #2). Next, ovsdb_idl_track_clear() will free the row, leaving the dangling pointer 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 <[email protected] > <mailto:[email protected]>> >> Fixes: 72aeb243a52a ("ovsdb-idl: Tracking - preserve data for deleted > rows.") > > It seems this commit wouldn't add any chance of use-after-free because > it made the condition more strict before freeing the row, while not > changing the logic of removing it from the hmap. If the use-after-free > is a real problem, it should have existed before the above commit. > Please correct me if I am wrong. > Sorry about this, I got confused by the different change tracking commits, it should probably be: Fixes: 932104f483ef ("ovsdb-idl: Add support for change tracking.") > Thanks, > Han Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
