diff mbox series

[ovs-dev,2/2] ovsdb-idl: Fix memory leak of ovsdb_idl_db_clear.

Message ID 1551838610-66580-2-git-send-email-hzhou8@ebay.com
State Superseded
Headers show
Series [ovs-dev,1/2] ovsdb-idl: Fix memory leak of ovsdb_idl_delete_row. | expand

Commit Message

Han Zhou March 6, 2019, 2:16 a.m. UTC 
From: Han Zhou <hzhou8@ebay.com>

ovsdb_idl_row_destroy() doesn't free the memory of row structure itself.
This is because of the ovsdb change tracking feature: the deleted row
may be accessed in the current iteration of main loop. The function
ovsdb_idl_row_destroy_postprocess() is called at the end of
ovsdb_idl_run() to free the deleted rows that are not tracked; the
function ovsdb_idl_db_track_clear() is called (indirectly) by user
at the end of each main loop iteration to free the deleted rows that
are tracked. However, in ovsdb_idl_db_clear(), which may be called when
a session is reset, or when the idl is destroyed, it didn't call
ovsdb_idl_row_destroy_postprocess(), which would result in all the
untracked rows leaked. This patch fixes that.

Signed-off-by: Han Zhou <hzhou8@ebay.com>
---
 lib/ovsdb-idl.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Ben Pfaff March 7, 2019, 10:22 p.m. UTC | #1 
When I apply this series I get lots of segfaults in the testsuite, e.g.:

=================================================================
==27505==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0x5557321b0f98 bp 0x7fffa7a2d230 sp 0x7fffa7a2d220 T0)
==27505==The signal is caused by a READ memory access.
==27505==Hint: address points to the zero page.
    #0 0x5557321b0f97 in hmap_remove ../include/openvswitch/hmap.h:287
    #1 0x5557321b0f97 in ovsdb_idl_row_destroy ../lib/ovsdb-idl.c:3106
    #2 0x5557321b1ac0 in ovsdb_idl_row_destroy ../lib/ovsdb-idl.c:3104
    #3 0x5557321b1ac0 in ovsdb_idl_row_clear_arcs ../lib/ovsdb-idl.c:3043
    #4 0x5557321b2198 in ovsdb_idl_delete_row ../lib/ovsdb-idl.c:3207
    #5 0x5557321b2e3a in ovsdb_idl_process_update2 ../lib/ovsdb-idl.c:2444
    #6 0x5557321b2e3a in ovsdb_idl_db_parse_update__ ../lib/ovsdb-idl.c:2304
    #7 0x5557321b2e3a in ovsdb_idl_db_parse_update ../lib/ovsdb-idl.c:2356
    #8 0x5557321b429a in ovsdb_idl_db_parse_update_rpc ../lib/ovsdb-idl.c:2187
    #9 0x5557321b557a in ovsdb_idl_db_parse_update_rpc ../lib/ovsdb-idl.c:2143
    #10 0x5557321b557a in ovsdb_idl_process_msg ../lib/ovsdb-idl.c:822
    #11 0x5557321b557a in ovsdb_idl_run ../lib/ovsdb-idl.c:901
    #12 0x5557321b557a in ovsdb_idl_run ../lib/ovsdb-idl.c:866
    #13 0x555731ed7015 in bridge_run ../vswitchd/bridge.c:2957
    #14 0x555731eb9464 in main ../vswitchd/ovs-vswitchd.c:125
    #15 0x7f4666eab09a in __libc_start_main ../csu/libc-start.c:308
    #16 0x555731ebbe99 in _start (/home/blp/nicira/ovs/_build/vswitchd/ovs-vswitchd+0x119e99)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ../include/openvswitch/hmap.h:287 in hmap_remove
==27505==ABORTING
Han Zhou March 8, 2019, 12:15 a.m. UTC | #2 
On Thu, Mar 7, 2019 at 2:22 PM Ben Pfaff <blp@ovn.org> wrote:
>
> When I apply this series I get lots of segfaults in the testsuite, e.g.:
>
> =================================================================
> ==27505==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0x5557321b0f98 bp 0x7fffa7a2d230 sp 0x7fffa7a2d220 T0)
> ==27505==The signal is caused by a READ memory access.
> ==27505==Hint: address points to the zero page.
>     #0 0x5557321b0f97 in hmap_remove ../include/openvswitch/hmap.h:287
>     #1 0x5557321b0f97 in ovsdb_idl_row_destroy ../lib/ovsdb-idl.c:3106
>     #2 0x5557321b1ac0 in ovsdb_idl_row_destroy ../lib/ovsdb-idl.c:3104
>     #3 0x5557321b1ac0 in ovsdb_idl_row_clear_arcs ../lib/ovsdb-idl.c:3043
>     #4 0x5557321b2198 in ovsdb_idl_delete_row ../lib/ovsdb-idl.c:3207
>     #5 0x5557321b2e3a in ovsdb_idl_process_update2 ../lib/ovsdb-idl.c:2444
>     #6 0x5557321b2e3a in ovsdb_idl_db_parse_update__ ../lib/ovsdb-idl.c:2304
>     #7 0x5557321b2e3a in ovsdb_idl_db_parse_update ../lib/ovsdb-idl.c:2356
>     #8 0x5557321b429a in ovsdb_idl_db_parse_update_rpc ../lib/ovsdb-idl.c:2187
>     #9 0x5557321b557a in ovsdb_idl_db_parse_update_rpc ../lib/ovsdb-idl.c:2143
>     #10 0x5557321b557a in ovsdb_idl_process_msg ../lib/ovsdb-idl.c:822
>     #11 0x5557321b557a in ovsdb_idl_run ../lib/ovsdb-idl.c:901
>     #12 0x5557321b557a in ovsdb_idl_run ../lib/ovsdb-idl.c:866
>     #13 0x555731ed7015 in bridge_run ../vswitchd/bridge.c:2957
>     #14 0x555731eb9464 in main ../vswitchd/ovs-vswitchd.c:125
>     #15 0x7f4666eab09a in __libc_start_main ../csu/libc-start.c:308
>     #16 0x555731ebbe99 in _start (/home/blp/nicira/ovs/_build/vswitchd/ovs-vswitchd+0x119e99)
>
> AddressSanitizer can not provide additional info.
> SUMMARY: AddressSanitizer: SEGV ../include/openvswitch/hmap.h:287 in hmap_remove
> ==27505==ABORTING

My apologies. It is caused by the first patch in this series. I can
reproduce easily by running idl tests such as: 2439: set, simple3
idl-compound-index-with-ref, initially populated - C FAILED
(ovsdb-idl.at:1811)

Not sure how I missed this in the first place. Probably I have run
irrelevant tests with "-k" and declared success. Anyways, stupid
mistake.

In fact the two patches in this series are independent. I ran test
with -k idl & -k ovn for patch 2/2 with & without address sanitizer,
and there is only one failure in ovn tests when running with address
sanitizer:

2745: ovn -- 1 LR with distributed router gateway port FAILED (ovn.at:8745)

However, this fails even with master code. Without address sanitizer
it doesn't fail. Running tests with address sanitizer is much slower,
so it could be timing problem in test cases. So I should say the patch
2/2 is ready for review.

I will figure out the problem of patch 1/2.

Thanks,
Han
Ben Pfaff March 8, 2019, 12:48 a.m. UTC | #3 
On Thu, Mar 07, 2019 at 04:15:20PM -0800, Han Zhou wrote:
> On Thu, Mar 7, 2019 at 2:22 PM Ben Pfaff <blp@ovn.org> wrote:
> My apologies. It is caused by the first patch in this series. I can
> reproduce easily by running idl tests such as: 2439: set, simple3
> idl-compound-index-with-ref, initially populated - C FAILED
> (ovsdb-idl.at:1811)
> 
> Not sure how I missed this in the first place. Probably I have run
> irrelevant tests with "-k" and declared success. Anyways, stupid
> mistake.

I do that sometimes too.

> In fact the two patches in this series are independent. I ran test
> with -k idl & -k ovn for patch 2/2 with & without address sanitizer,
> and there is only one failure in ovn tests when running with address
> sanitizer:
> 
> 2745: ovn -- 1 LR with distributed router gateway port FAILED (ovn.at:8745)
> 
> However, this fails even with master code. Without address sanitizer
> it doesn't fail. Running tests with address sanitizer is much slower,
> so it could be timing problem in test cases. So I should say the patch
> 2/2 is ready for review.

I do not get a failure with this test, running with (or presumably
without) Address Sanitizer, so I agree that it is likely a timing issue.

> I will figure out the problem of patch 1/2.

OK.  I will review patch 2.
Ben Pfaff March 8, 2019, 12:59 a.m. UTC | #4 
On Tue, Mar 05, 2019 at 06:16:50PM -0800, Han Zhou wrote:
> From: Han Zhou <hzhou8@ebay.com>
> 
> ovsdb_idl_row_destroy() doesn't free the memory of row structure itself.
> This is because of the ovsdb change tracking feature: the deleted row
> may be accessed in the current iteration of main loop. The function
> ovsdb_idl_row_destroy_postprocess() is called at the end of
> ovsdb_idl_run() to free the deleted rows that are not tracked; the
> function ovsdb_idl_db_track_clear() is called (indirectly) by user
> at the end of each main loop iteration to free the deleted rows that
> are tracked. However, in ovsdb_idl_db_clear(), which may be called when
> a session is reset, or when the idl is destroyed, it didn't call
> ovsdb_idl_row_destroy_postprocess(), which would result in all the
> untracked rows leaked. This patch fixes that.
> 
> Signed-off-by: Han Zhou <hzhou8@ebay.com>

Thanks.  I applied this patch (but not patch 1).
diff mbox series

Patch

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 49fd45c..2af123b 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -616,6 +616,7 @@  ovsdb_idl_db_clear(struct ovsdb_idl_db *db)
             ovsdb_idl_row_destroy(row);
         }
     }
+    ovsdb_idl_row_destroy_postprocess(db);
 
     db->cond_changed = false;
     db->cond_seqno = 0;