[ovs-dev] [PATCH ovn] logical-fields: fix memory leak caused by initialize ovnfield_by_name twice

[Mark Michelson]

On 2/28/20 5:40 AM, Damijan Skvarc wrote:
> Hi Mark
> 
> and thanks you made review of suggested patch.
> 
> just a few words of myself...
> I don't work professionally on this project, I started to look into this 
> code in my free hours just to satisfy my curiosity about how other 
> people work. I am following a couple of other projects just to learn 
> about different processes, tools and to keep my brains in good condition 
> while studying some others work. And if you get some things/knowledge it 
> is also kind to give back something. In case of ovs/ovn I found a nice 
> suite of automatic tests,. Unfortunately, by running them under valgrind 
> control the apparent reports become useless because of flood of memory 
> leaks. Mostly, these are small non-important leaks, however in a flood 
> of duplicated reports you can miss some serious issue. And here I found 
> my personal goal what I can give back. Unfortunately, I don't have  much 
> knowledge about networking, nor about original design. And if you don't 
> have that knowledge then from the experience I know it is better not to 
> touch anything for what you don't know why it has been designed for. So 
> every my patch is done in a way that nothing has been changed in a 
> design, just memory leak in valgrind reports disappears.

Yep, and contributions like yours are very much appreciated. If my 
criticism of your contribution came across as harsh, I apologize.

> 
> 
> On Thu, Feb 27, 2020 at 10:24 PM Mark Michelson <mmichels at redhat.com 
> <mailto:mmichels at redhat.com>> wrote:
> 
>     Hi Damijan,
> 
>     Thanks for finding the memory leak and patching it. To me,
>     ovnfield_by_name is a really strangely handled structure. There is an
>     explicit function to destroy the ovnfield_by_name structure, but its
>     creation is a side effect from ovn_symtab_init. The result is that
>     lflow.c calls ovn_symtab_init() and ovn_destroy_ovnfields(). But ofctrl
>     only calls ovn_symtab_init(). It doesn't call ovn_destroy_ovnfields()
>     because it would result in double-freeing ovnfield_by_name.
> 
> 
> yes, I noticed that asymmetry.
> 
>     This lack of symmetry shows a poor design.
> 
>     Your patch fixes the memory
>     leak, but it doesn't fix the lack of symmetry, and it still allows for
>     potential double-freeing. 
> 
>     I have a couple of suggestions for a better design:
> 
>     1) Instead of calling ovn_symbtab_init() in multiple places, call it
>     once in ovn_controller's main() function. Then when ovn_controller
>     exits, call ovn_destroy_ovnfields(). Then, pass the created symtab as a
>     parameter when it is necessary to. This works because the symtabs
>     created by lflow.c and ofctrl.c are identical, and their use of the
>     symtabs occurs in the same thread.
> 
>     2) Initialize ovnfield_by_name in a separate function from
>     ovn_symtab_init(). You can call this new ovnfield_by_name_init()
>     function in ovn-controller's main function and then call the already
>     existing ovn_destroy_ovnfields() function when ovn-controller exits.
>     This works because ovnfield_by_name does not rely on any data from the
>     created symtab. It exists completely independently.
> 
>     What do you think about this?
> 
> 
> What is strange also is that (If I see correctly) the ovnfield_by_name 
> is a hash of one single entry.
> I wouldn't use hash at all in this case. But since I don't know about 
> the design, probably some entries
> were deleted in the past and only one remained or probably there is a 
> plan some more entries will be
> added in the future. So let's assume we should not get rid of 
> ovnfield_by_name hash.
> 
> I found very similar functionality in meta_flow.c file (ovs). There 
> mf_by_name hash is more rational, it is filled with
> plenty of strings. Also initialization of this hash is more rational. It 
> is initialized only if it is needed (initialization is hidden
> in mf_from_name() and mf_from_name_len() functions and using the same 
> "pthread_once" approach as in my case). What is strange there is that 
> there is no destroy action for mf_by_name and valgrind does not report 
> any leak there. Probably test suite does not cover this functionality
That is an interesting data point. My guess is that the lack of a 
destructor for mf_by_name is an oversight. I'm not sure why valgrind 
doesn't report the leak.

> 
> I believe some more alternatives are available, however to reduce the 
> number of changes I would vote for
> - automatic and optional initialization similar as in meta_flow case
> - to solve asymmetry issue I would propose atexit() approach, similar as 
> it is implemented in ovs/lib/stopwatch.c

That's an excellent suggestion. That contains the allocation and 
destruction all within logical-fields.c without any other code having to 
know about its internals.

> btw, I didn't understand your first proposal. ovn_symbtab_init() 
> function need to be called twice since two different
> symtab entities are initialized. Is this probably also a design issue 
> that we could live with single symtab?

That's what I was thinking. Since the symtabs are identical and are 
treated as read-only (I think...), we could just have lflow and ofctrl 
share the same symtab. But I like your suggestion above better.

> 
> thanks,
> damijan
> 
>