[ovs-dev] [PATCH] route-table: Fix memory leak reported by valgrind.

[William Tu]

Hi Cascardo,

Thanks for your feedback.
I did a couple of more tests and I think it should be valgrind's false
positive. Even for testcase 1 (TESTSUITEFLAGS='1'), my valgrind
complains about this case as "possible lost."

On the other hand, I do check and make sure that we only called the
name_table_init() once. Although we never free the 'name_notifier',
since it's a static variable, valgrind should reports "still
reachable" instead of "possible lost" when ovs-vswitchd exits.

Regards,
William


On Mon, Jun 20, 2016 at 12:08 PM, Thadeu Lima de Souza Cascardo
<cascardo at redhat.com> wrote:
> On Mon, Jun 20, 2016 at 07:32:52AM -0700, William Tu wrote:
>> Testcase 2050, ovn -- 3 HVs, 1 LS, 3 lports/HV, reports possible leak:
>>     nln_notifier_create (netlink-notifier.c:131)
>>     name_table_init (route-table.c:319)
>>     route_table_init (route-table.c:110)
>>     dp_initialize (dpif.c:126)
>>     dp_unregister_provider (dpif.c:218)
>>     dpif_dummy_override (dpif-netdev.c:4309)
>>     dpif_dummy_register (dpif-netdev.c:4329)
>>     dummy_enable (dummy.c:46)
>>     parse_options (ovs-vswitchd.c:205)
>>     main (ovs-vswitchd.c:76)
>> 'name_notifier' could be overwritten without being free'd.
>>
>> Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/138910851
>> Signed-off-by: William Tu <u9012063 at gmail.com>
>> ---
>>  lib/route-table.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/lib/route-table.c b/lib/route-table.c
>> index 58e7f62..cf01c34 100644
>> --- a/lib/route-table.c
>> +++ b/lib/route-table.c
>> @@ -316,6 +316,7 @@ route_table_fallback_lookup(const struct in6_addr *ip6_dst OVS_UNUSED,
>>  static void
>>  name_table_init(void)
>>  {
>> +    free(name_notifier);
>>      name_notifier = rtnetlink_notifier_create(name_table_change, NULL);
>>  }
>
> That doesn't seem right. I could not reproduce the valgrind problem by running
> TESTSUITEFLAGS=2050 make check-valgrind.
>
> But this has several problems. Fist, it's not clear code. Second, if
> name_notifier was not NULL, it could release memory still in use, and cause
> other potential bugs and leaks as well.
>
> Third: it's not even necessary. This looks much more like a false positive from
> valgrind. Unless we are calling name_table_init twice, which I can't see how.
> Can you look into the real bug here, maybe WARN whenever name_table_init when
> name_notifier is not NULL?
>
> If this is really a false positive and you really want to get rid of it, you
> could just do:
>
>   static void
>   name_table_init(void)
>   {
>  -    name_notifier = rtnetlink_notifier_create(name_table_change, NULL);
>  +    if (name_notifier == NULL) {
>  +        name_notifier = rtnetlink_notifier_create(name_table_change, NULL);
>  +    }
>   }
>
> Cascardo.
>
>>
>> --
>> 2.5.0
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev