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

[William Tu]

Hi Ben,

Thanks, this indeed fixes the "possible" leaks.

Regards,
William


On Fri, Jul 1, 2016 at 8:24 PM, Ben Pfaff <blp at ovn.org> wrote:
> Hi William, please try this patch as a substitute for yours.  It should
> ensure that pointers to nln_notifiers are to the beginning of the
> structs instead of to the middle, meaning that valgrind does not
> consider them "possible" leaks.
>
> diff --git a/lib/netlink-notifier.c b/lib/netlink-notifier.c
> index 0867952..3de6e15 100644
> --- a/lib/netlink-notifier.c
> +++ b/lib/netlink-notifier.c
> @@ -46,9 +46,9 @@ struct nln {
>  };
>
>  struct nln_notifier {
> +    struct ovs_list node;
>      struct nln *nln;             /* Parent nln. */
>
> -    struct ovs_list node;
>      int multicast_group;         /* Multicast group we listen on. */
>      nln_notify_func *cb;
>      void *aux;
>
> On Sat, Jun 25, 2016 at 12:30:46PM -0700, William Tu wrote:
>> 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
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev