On Fri, Dec 15, 2023 at 7:24 PM Ilya Maximets <[email protected]> wrote: > > On 12/13/23 23:19, Frode Nordahl wrote: > > In the event a schema conversion aborts, the cleanup code in > > ovsdb_convert() prior to this patch will remove the in-memory > > copy of the new database prior to aborting any on-going > > transactions in that database, consequently leading to a use after > > free and potential crash. > > > > Fixes: 1b1d2e6daa56 ("ovsdb: Introduce experimental support for clustered > > databases.") > > Signed-off-by: Frode Nordahl <[email protected]> > > --- > > ovsdb/file.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/ovsdb/file.c b/ovsdb/file.c > > index 8bd1d4af3..778b4004b 100644 > > --- a/ovsdb/file.c > > +++ b/ovsdb/file.c > > @@ -388,10 +388,10 @@ ovsdb_convert(const struct ovsdb *src, const struct > > ovsdb_schema *new_schema, > > return NULL; > > > > error: > > - ovsdb_destroy(dst); > > if (txn) { > > ovsdb_txn_abort(txn); > > } > > + ovsdb_destroy(dst); > > *dstp = NULL; > > return error; > > } > > > Thanks, Frode! Good catch! > > Can we have a test case for this issue though? > I don't think we can test this directly, as the chances for the > crash are not 100%, but we can write a test and let asan fail it. > We do run tests under asan in CI, so that should be fine. > > The reproducer may look like this: > 1. Create a schema with 2 tables with string columns. > 2. Create database and add non-numerical string data to both tables. > 3. Try to convert the column in the first table to integer. > 4. The same for the other table. > > Steps 3 and 4 should expect failure, but should ignore the > error message, or at least ignore the table/column names in > the output, because we don't know which table will be first > during conversion. For the same reason we need both steps, > because we need transaction to be populated with the data > from one table before the conversion fails on the second one. > ASAN or valgrind should catch the UAF condition. > > What do you think?
Thanks alot for taking the time to review, much appreciated! A test case for this sounds like a good idea, I'll have a look. -- Frode Nordahl > > Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
