[PATCH wayland] tests: fix memory leak

[Marek Chalupa]

On 4 December 2014 at 12:12, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> 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?
>

In my branch I chose even different solution. Instead of doing it
differently in TEST and clients,
I did it differently in normal TESTs and sanity TESTs. Sanity tests are
currently the
only one that are not using wl_display_connect, so I added a macro
TEST_CLIENT_NOT_USING_WL_DISPLAY_CONNECT which just fixed the leak checks
for the sanity tests (or any other test that do not use
wl_display_connect() -- but there is none atm).


> > > 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?
>

Dunno, haven't tried anywhere else that on x86_64...
Anyway, in my development branch I added support for atomic
increasing/decreasing
of the counter.


>
> > 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
>

Still keep the patches on github, because the leak checker made the test to
fail
and I'd like to check if it is correct or I have a bug in the code
somewhere.
If you're (or anybody is) interested, it is here:

https://github.com/mchalupa/wayland/tree/tc-client-leaks


>
>
> Thanks,
> pq
>

Regards,
Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20141205/ca01e329/attachment-0001.html>