[ovs-dev,1/1] ovsdb-client: fix memory leak in do_needs_conversion() and do_convert()
diff mbox series

Message ID 1569831660-31998-1-git-send-email-damjan.skvarc@gmail.com
State New
Headers show
Series
  • [ovs-dev,1/1] ovsdb-client: fix memory leak in do_needs_conversion() and do_convert()
Related show

Commit Message

Damijan Skvarc Sept. 30, 2019, 8:21 a.m. UTC
Memory leak itself is not so important, however the problem is that
it is caused by forgetting to close rpc channel which might in
a long term lead to the leak of system resources.

Memory leak is reported by Valgrin running test suite and is expressed as:

==29472== 784 (600 direct, 184 indirect) bytes in 1 blocks are definitely lost in loss record 23 of 23
==29472==    at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==29472==    by 0x449D32: xcalloc (util.c:121)
==29472==    by 0x432147: jsonrpc_open (jsonrpc.c:87)
==29472==    by 0x40ABBE: open_jsonrpc (ovsdb-client.c:528)
==29472==    by 0x40ABBE: open_rpc (ovsdb-client.c:143)
==29472==    by 0x40AE50: do_needs_conversion (ovsdb-client.c:1670)
==29472==    by 0x405F76: main (ovsdb-client.c:282)

==29464== 784 (600 direct, 184 indirect) bytes in 1 blocks are definitely lost in loss record 23 of 23
==29464==    at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==29464==    by 0x449D32: xcalloc (util.c:121)
==29464==    by 0x432147: jsonrpc_open (jsonrpc.c:87)
==29464==    by 0x40ABBE: open_jsonrpc (ovsdb-client.c:528)
==29464==    by 0x40ABBE: open_rpc (ovsdb-client.c:143)
==29464==    by 0x40AF5A: do_convert (ovsdb-client.c:1644)
==29464==    by 0x405F76: main (ovsdb-client.c:282)

Signed-off-by: Damijan Skvarc <damjan.skvarc@gmail.com>
---
 ovsdb/ovsdb-client.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Ben Pfaff Sept. 30, 2019, 8:42 p.m. UTC | #1
On Mon, Sep 30, 2019 at 10:21:00AM +0200, Damijan Skvarc wrote:
> Memory leak itself is not so important, however the problem is that
> it is caused by forgetting to close rpc channel which might in
> a long term lead to the leak of system resources.
> 
> Memory leak is reported by Valgrin running test suite and is expressed as:

Thanks for the fix.

I don't understand why this is necessary.  These functions are called
from main(), which closes the rpc connection just afterward:

    command->handler(rpc, database, argc - optind, argv + optind);

    free(database);
    jsonrpc_close(rpc);

Can you take a closer look?
Damijan Skvarc Oct. 1, 2019, 8:23 a.m. UTC | #2
Hi Ben and thanks for review.

I double checked and I can confirm the patch is ok.
Changes which are applied to rpc pointer within do_convert() and
do_need_conversion() function are local to
functions and not propagated to outside world. This is because rpc is
passed to command handler functions as
pointer and reference to the pointer. Therefore main() could not know rpc
has been changed inside command
handler functions.

regards,Damijan.


On Mon, Sep 30, 2019 at 10:47 PM Ben Pfaff <blp@ovn.org> wrote:

> On Mon, Sep 30, 2019 at 10:21:00AM +0200, Damijan Skvarc wrote:
> > Memory leak itself is not so important, however the problem is that
> > it is caused by forgetting to close rpc channel which might in
> > a long term lead to the leak of system resources.
> >
> > Memory leak is reported by Valgrin running test suite and is expressed
> as:
>
> Thanks for the fix.
>
> I don't understand why this is necessary.  These functions are called
> from main(), which closes the rpc connection just afterward:
>
>     command->handler(rpc, database, argc - optind, argv + optind);
>
>     free(database);
>     jsonrpc_close(rpc);
>
> Can you take a closer look?
>
Ben Pfaff Oct. 1, 2019, 4:19 p.m. UTC | #3
You are right.  Thank you.  I applied this to master.

On Tue, Oct 01, 2019 at 10:23:45AM +0200, Damijan Skvarc wrote:
> Hi Ben and thanks for review.
> 
> I double checked and I can confirm the patch is ok.
> Changes which are applied to rpc pointer within do_convert() and
> do_need_conversion() function are local to
> functions and not propagated to outside world. This is because rpc is
> passed to command handler functions as
> pointer and reference to the pointer. Therefore main() could not know rpc
> has been changed inside command
> handler functions.
> 
> regards,Damijan.
> 
> 
> On Mon, Sep 30, 2019 at 10:47 PM Ben Pfaff <blp@ovn.org> wrote:
> 
> > On Mon, Sep 30, 2019 at 10:21:00AM +0200, Damijan Skvarc wrote:
> > > Memory leak itself is not so important, however the problem is that
> > > it is caused by forgetting to close rpc channel which might in
> > > a long term lead to the leak of system resources.
> > >
> > > Memory leak is reported by Valgrin running test suite and is expressed
> > as:
> >
> > Thanks for the fix.
> >
> > I don't understand why this is necessary.  These functions are called
> > from main(), which closes the rpc connection just afterward:
> >
> >     command->handler(rpc, database, argc - optind, argv + optind);
> >
> >     free(database);
> >     jsonrpc_close(rpc);
> >
> > Can you take a closer look?
> >

Patch
diff mbox series

diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
index e3ef372..e78e9d9 100644
--- a/ovsdb/ovsdb-client.c
+++ b/ovsdb/ovsdb-client.c
@@ -1657,6 +1657,7 @@  do_convert(struct jsonrpc *rpc, const char *database_ OVS_UNUSED,
     check_txn(jsonrpc_transact_block(rpc, request, &reply), &reply);
     jsonrpc_msg_destroy(reply);
     ovsdb_schema_destroy(new_schema);
+    jsonrpc_close(rpc);
 }
 
 static void
@@ -1677,6 +1678,7 @@  do_needs_conversion(struct jsonrpc *rpc, const char *database_ OVS_UNUSED,
     puts(ovsdb_schema_equal(schema1, schema2) ? "no" : "yes");
     ovsdb_schema_destroy(schema1);
     ovsdb_schema_destroy(schema2);
+    jsonrpc_close(rpc);
 }
 
 struct dump_table_aux {