[ovs-dev] [PATCH] ovsdb-idl: Adjust indexes during transactions.
Han Zhou
zhouhan at gmail.com
Thu Aug 16 03:56:21 UTC 2018
On Wed, Aug 15, 2018 at 8:53 PM, Han Zhou <zhouhan at gmail.com> wrote:
>
>
>
> On Tue, Aug 14, 2018 at 12:39 PM, Han Zhou <zhouhan at gmail.com> wrote:
> >
> >
> >
> > On Tue, Aug 14, 2018 at 11:31 AM, Ben Pfaff <blp at ovn.org> wrote:
> > >
> > > When transactions modified tables with indexes, the indexes were not
> > > properly updated to reflect the changes. For deleted rows, in
particular,
> > > this could cause use-after-free errors.
> > >
> > > This commit fixes the problem and adds a very simple test case
provided by
> > > Han Zhou that, without the fix, causes a crash.
> > >
> > > Reported-by: Han Zhou <zhouhan at gmail.com>
> > > Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-August/047185.html
> > > Signed-off-by: Ben Pfaff <blp at ovn.org>
> > > ---
> > > lib/ovsdb-idl.c | 25 +++++++++++++++++++++++--
> > > tests/test-ovsdb.c | 1 +
> > > 2 files changed, 24 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> > > index 8fdd18f4688e..a4d66113b18d 100644
> > > --- a/lib/ovsdb-idl.c
> > > +++ b/lib/ovsdb-idl.c
> > > @@ -3508,9 +3508,18 @@ ovsdb_idl_txn_disassemble(struct ovsdb_idl_txn
*txn)
> > > txn->db->txn = NULL;
> > >
> > > HMAP_FOR_EACH_SAFE (row, next, txn_node, &txn->txn_rows) {
> > > + enum { INSERTED, MODIFIED, DELETED } op
> > > + = (!row->new_datum ? DELETED
> > > + : !row->old_datum ? INSERTED
> > > + : MODIFIED);
> > > +
> > > + if (op != DELETED) {
> > > + ovsdb_idl_remove_from_indexes(row);
> > > + }
> > > +
> > > ovsdb_idl_destroy_all_map_op_lists(row);
> > > ovsdb_idl_destroy_all_set_op_lists(row);
> > > - if (row->old_datum) {
> > > + if (op != INSERTED) {
> > > if (row->written) {
> > > ovsdb_idl_row_unparse(row);
> > > ovsdb_idl_row_clear_arcs(row, false);
> > > @@ -3529,7 +3538,9 @@ ovsdb_idl_txn_disassemble(struct ovsdb_idl_txn
*txn)
> > >
> > > hmap_remove(&txn->txn_rows, &row->txn_node);
> > > hmap_node_nullify(&row->txn_node);
> > > - if (!row->old_datum) {
> > > + if (op != INSERTED) {
> > > + ovsdb_idl_add_to_indexes(row);
> > > + } else {
> > > hmap_remove(&row->table->rows, &row->hmap_node);
> > > free(row);
> > > }
> > > @@ -4209,6 +4220,10 @@ ovsdb_idl_txn_write__(const struct
ovsdb_idl_row *row_,
> > > goto discard_datum;
> > > }
> > >
> > > + bool index_row = is_index_row(row);
> > > + if (!index_row) {
> > > + ovsdb_idl_remove_from_indexes(row);
> > > + }
> > > if (hmap_node_is_null(&row->txn_node)) {
> > > hmap_insert(&row->table->db->txn->txn_rows, &row->txn_node,
> > > uuid_hash(&row->uuid));
> > > @@ -4231,6 +4246,9 @@ ovsdb_idl_txn_write__(const struct
ovsdb_idl_row *row_,
> > > }
> > > (column->unparse)(row);
> > > (column->parse)(row, &row->new_datum[column_idx]);
> > > + if (!index_row) {
> > > + ovsdb_idl_add_to_indexes(row);
> > > + }
> > > return;
> > >
> > > discard_datum:
> > > @@ -4358,6 +4376,8 @@ ovsdb_idl_txn_delete(const struct ovsdb_idl_row
*row_)
> > > }
> > >
> > > ovs_assert(row->new_datum != NULL);
> > > + ovs_assert(!is_index_row(row_));
> > > + ovsdb_idl_remove_from_indexes(row_);
> > > if (!row->old_datum) {
> > > ovsdb_idl_row_unparse(row);
> > > ovsdb_idl_row_clear_new(row);
> > > @@ -4405,6 +4425,7 @@ ovsdb_idl_txn_insert(struct ovsdb_idl_txn *txn,
> > > row->new_datum = xmalloc(class->n_columns * sizeof
*row->new_datum);
> > > hmap_insert(&row->table->rows, &row->hmap_node,
uuid_hash(&row->uuid));
> > > hmap_insert(&txn->txn_rows, &row->txn_node,
uuid_hash(&row->uuid));
> > > + ovsdb_idl_add_to_indexes(row);
> > > return row;
> > > }
> > >
> > > diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> > > index 793220400c3a..e981b588849d 100644
> > > --- a/tests/test-ovsdb.c
> > > +++ b/tests/test-ovsdb.c
> > > @@ -2882,6 +2882,7 @@ test_idl_compound_index_single_column(struct
ovsdb_idl *idl,
> > > ovs_assert(myRow->i == 4);
> > > txn = ovsdb_idl_txn_create(idl);
> > > idltest_simple_delete(myRow);
> > > + myRow = idltest_simple_index_find(i_index, toDelete);
> > > toInsert = idltest_simple_insert(txn);
> > > idltest_simple_set_i(toInsert, 54);
> > > idltest_simple_set_s(toInsert, "Lista054");
> > > --
> > > 2.16.1
> > >
> >
> > Thanks Ben for the quick fix!! It may be better if we add a little more
tests to ensure change is reflected in indexes before transaction commit.
> >
> > Acked-by: Han Zhou <hzhou8 at ebay.com>
>
> Hi Ben, I added below tests on top of your patch and it works as
expected. So consider it as my Tested-by, too.
>
> ----8><--------------------------------------------------><8---------
> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> index 459b4ee..4004cab 100644
> --- a/tests/test-ovsdb.c
> +++ b/tests/test-ovsdb.c
> @@ -2881,9 +2881,16 @@ test_idl_compound_index_single_column(struct
ovsdb_idl *idl,
> txn = ovsdb_idl_txn_create(idl);
> idltest_simple_delete(myRow);
> myRow = idltest_simple_index_find(i_index, toDelete);
> - toInsert = idltest_simple_insert(txn);
> - idltest_simple_set_i(toInsert, 54);
> - idltest_simple_set_s(toInsert, "Lista054");
> + ovs_assert(!myRow);
> + myRow = idltest_simple_insert(txn);
> + idltest_simple_set_i(myRow, 54);
> + idltest_simple_set_s(myRow, "Lista054");
> + toInsert = idltest_simple_index_init_row(i_index);
> + idltest_simple_index_set_i(toInsert, 54);
> + myRow = idltest_simple_index_find(i_index, toInsert);
> + ovs_assert(myRow);
> + ovs_assert(myRow->i == 54);
> + ovs_assert(!strcmp(myRow->s, "Lista054"));
> ovsdb_idl_txn_commit_block(txn);
> ovsdb_idl_txn_destroy(txn);
> idltest_simple_index_set_i(to, 60);
> @@ -2905,6 +2912,8 @@ test_idl_compound_index_single_column(struct
ovsdb_idl *idl,
> ovs_assert(myRow->i == 10);
> txn = ovsdb_idl_txn_create(idl);
> idltest_simple_set_i(myRow, 30);
> + myRow = idltest_simple_index_find(i_index, toUpdate);
> + ovs_assert(!myRow);
> ovsdb_idl_txn_commit_block(txn);
> ovsdb_idl_txn_destroy(txn);
> idltest_simple_index_set_i(to, 60);
>
Sorry, I forgot to destroy the new index row. Below is the corrected
version:
----8><--------------------------------------------------><8---------
diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index 459b4ee..453d88e 100644
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -2881,9 +2881,16 @@ test_idl_compound_index_single_column(struct
ovsdb_idl *idl,
txn = ovsdb_idl_txn_create(idl);
idltest_simple_delete(myRow);
myRow = idltest_simple_index_find(i_index, toDelete);
- toInsert = idltest_simple_insert(txn);
- idltest_simple_set_i(toInsert, 54);
- idltest_simple_set_s(toInsert, "Lista054");
+ ovs_assert(!myRow);
+ myRow = idltest_simple_insert(txn);
+ idltest_simple_set_i(myRow, 54);
+ idltest_simple_set_s(myRow, "Lista054");
+ toInsert = idltest_simple_index_init_row(i_index);
+ idltest_simple_index_set_i(toInsert, 54);
+ myRow = idltest_simple_index_find(i_index, toInsert);
+ ovs_assert(myRow);
+ ovs_assert(myRow->i == 54);
+ ovs_assert(!strcmp(myRow->s, "Lista054"));
ovsdb_idl_txn_commit_block(txn);
ovsdb_idl_txn_destroy(txn);
idltest_simple_index_set_i(to, 60);
@@ -2905,6 +2912,8 @@ test_idl_compound_index_single_column(struct
ovsdb_idl *idl,
ovs_assert(myRow->i == 10);
txn = ovsdb_idl_txn_create(idl);
idltest_simple_set_i(myRow, 30);
+ myRow = idltest_simple_index_find(i_index, toUpdate);
+ ovs_assert(!myRow);
ovsdb_idl_txn_commit_block(txn);
ovsdb_idl_txn_destroy(txn);
idltest_simple_index_set_i(to, 60);
@@ -2928,6 +2937,7 @@ test_idl_compound_index_single_column(struct
ovsdb_idl *idl,
idltest_simple_index_destroy_row(to);
idltest_simple_index_destroy_row(equal);
idltest_simple_index_destroy_row(toDelete);
+ idltest_simple_index_destroy_row(toInsert);
idltest_simple_index_destroy_row(toUpdate);
return step;
}
More information about the dev
mailing list