[ovs-dev] [PATCH 1/1] ovsdb-server: fix memory leak in ovsdb_convert_table() function.

Ben Pfaff blp at ovn.org
Thu Oct 24 19:48:45 UTC 2019


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 at 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.


More information about the dev mailing list