| Message ID | 1571901649-5501-1-git-send-email-damjan.skvarc@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,1/1] ovsdb-server: fix memory leak in ovsdb_convert_table() function. | expand |
Bleep bloop. Greetings Damijan Skvarc, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 88 characters long (recommended limit is 79)
#43 FILE: ovsdb/file.c:238:
ovsdb_datum_destroy(&dst_row->fields[dst_column->index], &dst_column->type);
Lines checked: 50, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On Thu, Oct 24, 2019 at 09:20:49AM +0200, Damijan Skvarc wrote: > Memory leak happens while converting existing database into new database according to > the specified schema (ovsdb-client convert new-schema). Memory leak was detected > by valgrind while executing functional test "schema conversion online - clustered" > > ==16202== 96 bytes in 6 blocks are definitely lost in loss record 326 of 399 > ==16202== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==16202== by 0x44A5D4: xmalloc (util.c:138) > ==16202== by 0x4377A6: alloc_default_atoms (ovsdb-data.c:315) > ==16202== by 0x437F18: ovsdb_datum_init_default (ovsdb-data.c:918) > ==16202== by 0x413D82: ovsdb_row_create (row.c:59) > ==16202== by 0x40AA53: ovsdb_convert_table (file.c:220) > ==16202== by 0x40AA53: ovsdb_convert (file.c:275) > ==16202== by 0x416BE1: ovsdb_trigger_try (trigger.c:255) > ==16202== by 0x40D29E: ovsdb_jsonrpc_trigger_create (jsonrpc-server.c:1119) > ==16202== by 0x40D29E: ovsdb_jsonrpc_session_got_request (jsonrpc-server.c:986) > ==16202== by 0x40D29E: ovsdb_jsonrpc_session_run (jsonrpc-server.c:556) > ==16202== by 0x40D29E: ovsdb_jsonrpc_session_run_all (jsonrpc-server.c:586) > ==16202== by 0x40D29E: ovsdb_jsonrpc_server_run (jsonrpc-server.c:401) > ==16202== by 0x40682E: main_loop (ovsdb-server.c:209) > ==16202== by 0x40682E: main (ovsdb-server.c:460) > > The problem was in ovsdb_datum_convert() function, which overrides pointers to datum memory allocated in > ovsdb_row_create() function. Fix was done by freeing this memory before ovsdb_datum_convert() is called. > > Signed-off-by: Damijan Skvarc <damjan.skvarc@gmail.com> Thanks a lot for finding the memory leak. I think that this adds a risk of bad memory access on the error path, because ovsdb_datum_convert() leaves its destination datum indeterminate on error. I think that the following cures that: diff --git a/ovsdb/file.c b/ovsdb/file.c index 7ac7df0510f0..22f89a39791b 100644 --- a/ovsdb/file.c +++ b/ovsdb/file.c @@ -241,6 +241,7 @@ ovsdb_convert_table(struct ovsdb_txn *txn, &dst_row->fields[dst_column->index], &dst_column->type, &src_row->fields[src_column->index], &src_column->type); if (error) { + ovsdb_datum_init_empty(&dst_row->fields[dst_column->index]); ovsdb_row_destroy(dst_row); return error; } Would you please check my work on that and make sure it still does the job and submit a v2? Thanks very much.
diff --git a/ovsdb/file.c b/ovsdb/file.c index 8d16b09..7ac7df0 100644 --- a/ovsdb/file.c +++ b/ovsdb/file.c @@ -235,6 +235,8 @@ ovsdb_convert_table(struct ovsdb_txn *txn, continue; } + ovsdb_datum_destroy(&dst_row->fields[dst_column->index], &dst_column->type); + struct ovsdb_error *error = ovsdb_datum_convert( &dst_row->fields[dst_column->index], &dst_column->type, &src_row->fields[src_column->index], &src_column->type);
Memory leak happens while converting existing database into new database according to the specified schema (ovsdb-client convert new-schema). Memory leak was detected by valgrind while executing functional test "schema conversion online - clustered" ==16202== 96 bytes in 6 blocks are definitely lost in loss record 326 of 399 ==16202== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==16202== by 0x44A5D4: xmalloc (util.c:138) ==16202== by 0x4377A6: alloc_default_atoms (ovsdb-data.c:315) ==16202== by 0x437F18: ovsdb_datum_init_default (ovsdb-data.c:918) ==16202== by 0x413D82: ovsdb_row_create (row.c:59) ==16202== by 0x40AA53: ovsdb_convert_table (file.c:220) ==16202== by 0x40AA53: ovsdb_convert (file.c:275) ==16202== by 0x416BE1: ovsdb_trigger_try (trigger.c:255) ==16202== by 0x40D29E: ovsdb_jsonrpc_trigger_create (jsonrpc-server.c:1119) ==16202== by 0x40D29E: ovsdb_jsonrpc_session_got_request (jsonrpc-server.c:986) ==16202== by 0x40D29E: ovsdb_jsonrpc_session_run (jsonrpc-server.c:556) ==16202== by 0x40D29E: ovsdb_jsonrpc_session_run_all (jsonrpc-server.c:586) ==16202== by 0x40D29E: ovsdb_jsonrpc_server_run (jsonrpc-server.c:401) ==16202== by 0x40682E: main_loop (ovsdb-server.c:209) ==16202== by 0x40682E: main (ovsdb-server.c:460) The problem was in ovsdb_datum_convert() function, which overrides pointers to datum memory allocated in ovsdb_row_create() function. Fix was done by freeing this memory before ovsdb_datum_convert() is called. Signed-off-by: Damijan Skvarc <damjan.skvarc@gmail.com> --- ovsdb/file.c | 2 ++ 1 file changed, 2 insertions(+)