[PATCH wayland] tests: fix memory leak

[Pekka Paalanen]

On Wed, 3 Dec 2014 11:44:47 +0100
Marek Chalupa <mchqwerty at gmail.com> wrote:

> On 1 December 2014 at 11:42, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 
> > On Fri, 21 Nov 2014 11:18:33 +0100
> > Marek Chalupa <mchqwerty at gmail.com> wrote:
> >
> > > We didn't free the struct client that we got from client_connect()
> > >
> > > Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
> > > ---
> > >  tests/display-test.c    | 23 +++++++++++++----------
> > >  tests/test-compositor.c |  1 +
> > >  2 files changed, 14 insertions(+), 10 deletions(-)
> >
> > Hi,
> >
> > how did you find these leaks?
> >
> 
> By valgrind --trace-children=yes
> 
> 
> >
> > More importantly, why doesn't our leak checker find these?
> > That is, the code in test-runner.c, run_test().
> >
> 
> Because it is in another fork. It looks like this:
> 
> run_test()
>      int the alloc_count  and fds_count
>      run the TEST function
>           create_client --> fork
>                 [in fork]   alloc, dealloc, whatever
>           wait for client
>      check alloc_count and fds_count
> 
> The problem is that the forked client has the alloc_count and fds_count
> in its own virtual memory, so the leak checker finds only leaks in
> TEST function.
> 
> I have some patches that extend these leak checks
> into the client too, but I ran into problem with filedescriptors there.
> The problem is that when forking the client, we pass WAYLAND_SOCKET fd
> to wl_display_connect(). wl_display_connect() just uses this fd (no dup),
> so wl_display_disconnect() closes this fd.
> So the result of leak checker is that the client closed
> one more fd that it opened. Hardcoding -1 to client's fds number
> is not a solution too, because client does not have to call
> wl_display_connect()
> 
> So summing it up: using leak checker in clients is possible, but only for
> memory
> leaks. For fd leaks we'd have to use some more sophisticated solution.

Erf, more complicated than I imagined.

I think the fd leak thing should be easy to fix though: I'm not sure
the strict equality check for open fds makes too much sense, it would
be also good if there are less fds open in the end than in the
beginning. Right? So fail only if in the end there are more fds open.

That seems more useful to me than not doing fd leak checks at all.
Maybe we can even do it differently in TEST vs. clients?

> > For one, I think those are not thread-safe. Calling exit() in a test
> > would bypass it too, but I'm not sure that happens.
> >
> 
> Huh? What is not thread-safe? you mean leak checks when running only one
> test (without fork?)

I mean e.g. this function

__attribute__ ((visibility("default"))) void *
malloc(size_t size)
{
        num_alloc++;
        return sys_malloc(size);
}

is not thread-safe, because it read-modify-writes a global variable
without any explicit synchronization guarantees. It's not using a mutex
nor a guaranteed-atomic variable or operation. We just get lucky on
most arches, I assume. Or at least on x86?

> Yes, it looks like when calling exit in the test, the leak check is
> bypassed.
> Quick grep though the tests shows that there are no calls to exit in TEST,
> just in
> the forked clients (which is not a problem _now_ with no leak checks in
> clients)

Yeah.

> > The Valgrind output nowadays is quite messy. Even when I run a single
> > test from one test binary which should not fork at all, I think I see
> > two forks in Valgrind output: one is likely the debugger check, maybe
> > the other is actually a thread for the test compositor or something?
> >
> 
> Yes, the first fork is the debugger check and the other is the client
> for test compositor. In the tests without the test compositor you should
> see only
> one fork (the debugger check)

I suppose we need to live with that.

> > I think Valgrind is saying that there are many more leaks to plug.
> >
> >
> Yes


Thanks,
pq