| Message ID | 20180814183146.30858-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovsdb-idl: Adjust indexes during transactions. | expand |
On Tue, Aug 14, 2018 at 11:31 AM, Ben Pfaff <blp@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@gmail.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-August/047185.html > Signed-off-by: Ben Pfaff <blp@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@ebay.com>
On Tue, Aug 14, 2018 at 12:39 PM, Han Zhou <zhouhan@gmail.com> wrote: > > > > On Tue, Aug 14, 2018 at 11:31 AM, Ben Pfaff <blp@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@gmail.com> > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-August/047185.html > > Signed-off-by: Ben Pfaff <blp@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@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);
On Wed, Aug 15, 2018 at 8:53 PM, Han Zhou <zhouhan@gmail.com> wrote: > > > > On Tue, Aug 14, 2018 at 12:39 PM, Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > On Tue, Aug 14, 2018 at 11:31 AM, Ben Pfaff <blp@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@gmail.com> > > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-August/047185.html > > > Signed-off-by: Ben Pfaff <blp@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@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; }
On Wed, Aug 15, 2018 at 08:56:21PM -0700, Han Zhou wrote: > On Wed, Aug 15, 2018 at 8:53 PM, Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > On Tue, Aug 14, 2018 at 12:39 PM, Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > > > > > On Tue, Aug 14, 2018 at 11:31 AM, Ben Pfaff <blp@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@gmail.com> > > > > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-discuss/2018-August/047185.html > > > > Signed-off-by: Ben Pfaff <blp@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@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. Thanks a lot for the test improvements. I applied this, with the tests, to master and branch-2.10.
On Thu, Aug 16, 2018 at 10:39 AM, Ben Pfaff <blp@ovn.org> wrote: > > On Wed, Aug 15, 2018 at 08:56:21PM -0700, Han Zhou wrote: > > On Wed, Aug 15, 2018 at 8:53 PM, Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > > > > > On Tue, Aug 14, 2018 at 12:39 PM, Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > > > > > > > > > On Tue, Aug 14, 2018 at 11:31 AM, Ben Pfaff <blp@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@gmail.com> > > > > > Reported-at: > > https://mail.openvswitch.org/pipermail/ovs-discuss/2018-August/047185.html > > > > > Signed-off-by: Ben Pfaff <blp@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@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. > > Thanks a lot for the test improvements. > > I applied this, with the tests, to master and branch-2.10. Thanks Ben. I think it is needed in 2.9 and 2.8, too.
On Thu, Aug 16, 2018 at 10:40:23AM -0700, Han Zhou wrote: > On Thu, Aug 16, 2018 at 10:39 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > On Wed, Aug 15, 2018 at 08:56:21PM -0700, Han Zhou wrote: > > > On Wed, Aug 15, 2018 at 8:53 PM, Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > > > > > > > > > On Tue, Aug 14, 2018 at 12:39 PM, Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > On Tue, Aug 14, 2018 at 11:31 AM, Ben Pfaff <blp@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@gmail.com> > > > > > > Reported-at: > > > > https://mail.openvswitch.org/pipermail/ovs-discuss/2018-August/047185.html > > > > > > Signed-off-by: Ben Pfaff <blp@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@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. > > > > Thanks a lot for the test improvements. > > > > I applied this, with the tests, to master and branch-2.10. > > Thanks Ben. I think it is needed in 2.9 and 2.8, too. The backport to 2.9 was pretty easy so I did it. 2.8 seems to lack a lot of related stuff. I don't think we really used indexes in that release, so I don't think the backport is worth it.
On Thu, Aug 16, 2018 at 10:50 AM, Ben Pfaff <blp@ovn.org> wrote: > > On Thu, Aug 16, 2018 at 10:40:23AM -0700, Han Zhou wrote: > > On Thu, Aug 16, 2018 at 10:39 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > > > On Wed, Aug 15, 2018 at 08:56:21PM -0700, Han Zhou wrote: > > > > On Wed, Aug 15, 2018 at 8:53 PM, Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > On Tue, Aug 14, 2018 at 12:39 PM, Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Aug 14, 2018 at 11:31 AM, Ben Pfaff <blp@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@gmail.com> > > > > > > > Reported-at: > > > > > > https://mail.openvswitch.org/pipermail/ovs-discuss/2018-August/047185.html > > > > > > > Signed-off-by: Ben Pfaff <blp@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@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. > > > > > > Thanks a lot for the test improvements. > > > > > > I applied this, with the tests, to master and branch-2.10. > > > > Thanks Ben. I think it is needed in 2.9 and 2.8, too. > > The backport to 2.9 was pretty easy so I did it. > > 2.8 seems to lack a lot of related stuff. I don't think we really used > indexes in that release, so I don't think the backport is worth it. Agreed. 2.9 is good enough. Thanks!
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");
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@gmail.com> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-August/047185.html Signed-off-by: Ben Pfaff <blp@ovn.org> --- lib/ovsdb-idl.c | 25 +++++++++++++++++++++++-- tests/test-ovsdb.c | 1 + 2 files changed, 24 insertions(+), 2 deletions(-)