| Message ID | 20220202145144.25049-1-dceara@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ovsdb-idl: Fix use-after-free when destroying an IDL loop. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 2/2/22 15:51, Dumitru Ceara wrote: > Transactions that are still incomplete (waiting for a reply from the > server) are kept in the IDL's 'outstanding_txns' map. When a transaction > is destroyed, ovsdb_idl_txn_destroy() will take care of removing the > transaction from the 'outstanding_txns' map if the transaction was > incomplete. > > When destroying the IDL loop, instead of trying to abort the > committing_txn which would set the state to "aborted", just disassemble > it, such that ovsdb_idl_txn_destroy() properly cleans it up. > > Fixes: 53a540e5311c ("ovsdb-idl: ovsdb_idl_loop_destroy must also destroy the committing txn.") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > lib/ovsdb-idl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index 46f51a527356..9064baa88a4b 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -4243,7 +4243,7 @@ ovsdb_idl_loop_destroy(struct ovsdb_idl_loop *loop) > { > if (loop) { > if (loop->committing_txn) { > - ovsdb_idl_txn_abort(loop->committing_txn); > + ovsdb_idl_txn_disassemble(loop->committing_txn); Hmm. Do we really need this call? txn_destroy will remove the incomplete transaction from the outstanding_txns and abort it, i.e. disassemble, right after that. > ovsdb_idl_txn_destroy(loop->committing_txn); > } > ovsdb_idl_destroy(loop->idl);
On 2/4/22 12:14, Ilya Maximets wrote: > On 2/2/22 15:51, Dumitru Ceara wrote: >> Transactions that are still incomplete (waiting for a reply from the >> server) are kept in the IDL's 'outstanding_txns' map. When a transaction >> is destroyed, ovsdb_idl_txn_destroy() will take care of removing the >> transaction from the 'outstanding_txns' map if the transaction was >> incomplete. >> >> When destroying the IDL loop, instead of trying to abort the >> committing_txn which would set the state to "aborted", just disassemble >> it, such that ovsdb_idl_txn_destroy() properly cleans it up. >> >> Fixes: 53a540e5311c ("ovsdb-idl: ovsdb_idl_loop_destroy must also destroy the committing txn.") >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> lib/ovsdb-idl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c >> index 46f51a527356..9064baa88a4b 100644 >> --- a/lib/ovsdb-idl.c >> +++ b/lib/ovsdb-idl.c >> @@ -4243,7 +4243,7 @@ ovsdb_idl_loop_destroy(struct ovsdb_idl_loop *loop) >> { >> if (loop) { >> if (loop->committing_txn) { >> - ovsdb_idl_txn_abort(loop->committing_txn); >> + ovsdb_idl_txn_disassemble(loop->committing_txn); > > Hmm. Do we really need this call? txn_destroy will remove the incomplete > transaction from the outstanding_txns and abort it, i.e. disassemble, right > after that. > That's a very good point, I missed that completely, I'll send a v2. Regards, Dumitru
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index 46f51a527356..9064baa88a4b 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -4243,7 +4243,7 @@ ovsdb_idl_loop_destroy(struct ovsdb_idl_loop *loop) { if (loop) { if (loop->committing_txn) { - ovsdb_idl_txn_abort(loop->committing_txn); + ovsdb_idl_txn_disassemble(loop->committing_txn); ovsdb_idl_txn_destroy(loop->committing_txn); } ovsdb_idl_destroy(loop->idl);
Transactions that are still incomplete (waiting for a reply from the server) are kept in the IDL's 'outstanding_txns' map. When a transaction is destroyed, ovsdb_idl_txn_destroy() will take care of removing the transaction from the 'outstanding_txns' map if the transaction was incomplete. When destroying the IDL loop, instead of trying to abort the committing_txn which would set the state to "aborted", just disassemble it, such that ovsdb_idl_txn_destroy() properly cleans it up. Fixes: 53a540e5311c ("ovsdb-idl: ovsdb_idl_loop_destroy must also destroy the committing txn.") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- lib/ovsdb-idl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)