On Wed, Jun 7, 2023 at 6:07 AM Ilya Maximets <[email protected]> wrote: > > Initial change set is preserved for as long as the monitor itself. > However, if a new client has a condition on a column that is not > one of the monitored columns, this column will be added to the > monitor via ovsdb_monitor_condition_bind(). This new column, however, > doesn't exist in the initial change set. That will cause ovsdb-server > to malfunction or crash trying to access non-existent column during > condition evaluation: > > ERROR: AddressSanitizer: heap-buffer-overflow > READ of size 4 at 0x606000006780 thread T0 > 0 ovsdb_clause_evaluate ovsdb/condition.c:328:26 > 1 ovsdb_condition_match_any_clause ovsdb/condition.c:441:13 > 2 ovsdb_condition_empty_or_match_any ovsdb/condition.h:84:13 > 3 ovsdb_monitor_row_update_type_condition ovsdb/monitor.c:892:28 > 4 ovsdb_monitor_compose_row_update2 ovsdb/monitor.c:1058:12 > 5 ovsdb_monitor_compose_update ovsdb/monitor.c:1172:24 > 6 ovsdb_monitor_get_update ovsdb/monitor.c:1276:24 > 7 ovsdb_jsonrpc_monitor_create ovsdb/jsonrpc-server.c:1505:12 > 8 ovsdb_jsonrpc_session_got_request ovsdb/jsonrpc-server.c:1030:21 > 9 ovsdb_jsonrpc_session_run ovsdb/jsonrpc-server.c:572:17 > 10 ovsdb_jsonrpc_session_run_all ovsdb/jsonrpc-server.c:602:21 > 11 ovsdb_jsonrpc_server_run ovsdb/jsonrpc-server.c:417:9 > 12 main_loop ovsdb/ovsdb-server.c:222:9 > 13 main ovsdb/ovsdb-server.c:500:5 > 14 __libc_start_call_main > 15 __libc_start_main@GLIBC_2.2.5 > 16 _start (ovsdb/ovsdb-server+0x473034) > > Located 0 bytes after 64-byte region [0x606000006740,0x606000006780) > allocated by thread T0 here: > 0 malloc (ovsdb/ovsdb-server+0x50dc82) > 1 xmalloc__ lib/util.c:140:15 > 2 xmalloc lib/util.c:175:12 > 3 clone_monitor_row_data ovsdb/monitor.c:336:12 > 4 ovsdb_monitor_changes_update ovsdb/monitor.c:1384:23 > 5 ovsdb_monitor_get_initial ovsdb/monitor.c:1535:21 > 6 ovsdb_jsonrpc_monitor_create ovsdb/jsonrpc-server.c:1502:9 > 7 ovsdb_jsonrpc_session_got_request ovsdb/jsonrpc-server.c:1030:21 > 8 ovsdb_jsonrpc_session_run ovsdb/jsonrpc-server.c:572:17 > 9 ovsdb_jsonrpc_session_run_all ovsdb/jsonrpc-server.c:602:21 > 10 ovsdb_jsonrpc_server_run ovsdb/jsonrpc-server.c:417:9 > 11 main_loop ovsdb/ovsdb-server.c:222:9 > 12 main ovsdb/ovsdb-server.c:500:5 > 13 __libc_start_call_main > 14 __libc_start_main@GLIBC_2.2.5 > 15 _start (ovsdb/ovsdb-server+0x473034) > > Fix that by destroying the initial change set every time new columns > are added to the monitor. This will trigger re-generation of the > change set and it will contain all the necessary columns afterwards. > > Fixes: 07c27226ee96 ("ovsdb: Monitor: Keep and maintain the initial change set.") > Reported-by: Han Zhou <[email protected]> > Signed-off-by: Ilya Maximets <[email protected]> > --- > ovsdb/monitor.c | 15 +++++++++- > tests/ovsdb-monitor.at | 66 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 80 insertions(+), 1 deletion(-) > > diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c > index 04dcd2298..4afaa89f4 100644 > --- a/ovsdb/monitor.c > +++ b/ovsdb/monitor.c > @@ -478,6 +478,7 @@ ovsdb_monitor_add_column(struct ovsdb_monitor *dbmon, > enum ovsdb_monitor_selection select, > bool monitored) > { > + struct ovsdb_monitor_change_set *mcs; > struct ovsdb_monitor_table *mt; > struct ovsdb_monitor_column *c; > > @@ -488,6 +489,18 @@ ovsdb_monitor_add_column(struct ovsdb_monitor *dbmon, > return column->name; > } > > + mcs = dbmon->init_change_set; > + if (mcs) { > + /* A new column is going to be added to the monitor. Existing > + * initial change set doesn't have it, so can no longer be used. > + * Initial change set is never used by more than one session at > + * the same time, so it's safe to destroy it here. */ > + ovs_assert(mcs->n_refs == 1); > + ovsdb_monitor_json_cache_destroy(dbmon, mcs); > + ovsdb_monitor_change_set_destroy(mcs); > + dbmon->init_change_set = NULL; > + } > + > if (mt->n_columns >= mt->allocated_columns) { > mt->columns = x2nrealloc(mt->columns, &mt->allocated_columns, > sizeof *mt->columns); > @@ -614,7 +627,7 @@ ovsdb_monitor_untrack_change_set(struct ovsdb_monitor *dbmon, > if (--mcs->n_refs == 0) { > if (mcs == dbmon->init_change_set) { > /* The initial change set should exist as long as the > - * monitor itself. */ > + * monitor doesn't change. */ > mcs->n_refs++; > return; > } else if (mcs == dbmon->new_change_set) { > diff --git a/tests/ovsdb-monitor.at b/tests/ovsdb-monitor.at > index 7e1ff64f0..12cd2bc31 100644 > --- a/tests/ovsdb-monitor.at > +++ b/tests/ovsdb-monitor.at > @@ -1011,3 +1011,69 @@ row,action,name,number,_version > ]], [ignore]) > AT_CLEANUP > > +AT_SETUP([monitor-cond initial reply with condition on non-monitored column]) > +AT_KEYWORDS([ovsdb server monitor monitor-cond positive initial non-monitored]) > + > +ordinal_schema > schema > +AT_CHECK([ovsdb-tool create db schema], [0], [stdout], [ignore]) > +on_exit 'kill `cat ovsdb-server.pid`' > +AT_CAPTURE_FILE([ovsdb-server.log]) > +AT_CHECK([ovsdb-server --detach --no-chdir --pidfile \ > + --remote=punix:socket --log-file db], [0], [ignore], [ignore]) > + > +dnl Initialize the database content. > +for txn in m4_foreach([txn], [[[["ordinals", > + {"op": "insert", > + "table": "ordinals", > + "row": {"number": 0, "name": "zero"}}, > + {"op": "insert", > + "table": "ordinals", > + "row": {"number": 1, "name": "one"}}, > + {"op": "insert", > + "table": "ordinals", > + "row": {"number": 2, "name": "two"}}]]]], ['txn' ]); do > + AT_CHECK([ovsdb-client transact unix:socket "$txn"], [0], [ignore], [ignore]) > +done > + > +dnl Start a first client that monitors only the column 'name'. > +on_exit 'kill `cat client-1.pid`' > +AT_CAPTURE_FILE([client-1.out]) > +AT_CHECK([ovsdb-client -vjsonrpc --pidfile=client-1.pid --detach --no-chdir \ > + -d json monitor-cond --format=csv unix:socket \ > + ordinals '[[true]]' ordinals ["name"] \ > + > client-1.out 2> client-1.err], [0], [ignore], [ignore]) > +dnl Wait for the initial monitor reply. > +OVS_WAIT_UNTIL([grep -q 'initial' client-1.out]) > + > +dnl Start a second client that monitors the column 'name', but has a condition > +dnl on column 'number'. > +on_exit 'kill `cat client-2.pid`' > +AT_CAPTURE_FILE([client-2.out]) > +AT_CHECK([ovsdb-client -vjsonrpc --pidfile=client-2.pid --detach --no-chdir \ > + -d json monitor-cond --format=csv unix:socket \ > + ordinals '[[["number", "!=", 1]]]' ordinals ["name"] \ > + > client-2.out 2> client-2.err], [0], [ignore], [ignore]) > +dnl Wait for the initial monitor reply. > +OVS_WAIT_UNTIL([grep -q 'initial' client-2.out]) > + > +OVSDB_SERVER_SHUTDOWN > +OVS_WAIT_UNTIL([test ! -e ovsdb-server.pid && \ > + test ! -e client-1.pid && test ! -e client-2.pid]) > + > +dnl The first client should have all the names. > +AT_CHECK([$PYTHON3 $srcdir/ovsdb-monitor-sort.py < client-1.out | uuidfilt], > + [0], [dnl > +row,action,name > +<0>,initial,"""one""" > +<1>,initial,"""two""" > +<2>,initial,"""zero""" > +]) > + > +dnl The second client should not have the name 'one'. > +AT_CHECK([$PYTHON3 $srcdir/ovsdb-monitor-sort.py < client-2.out | uuidfilt], > + [0], [dnl > +row,action,name > +<0>,initial,"""two""" > +<1>,initial,"""zero""" > +]) > +AT_CLEANUP > -- > 2.40.1 >
Thanks Ilya. Acked-by: Han Zhou <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
