1055322 - libnestegg leaks about 1 byte per pool_ctx

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Andrew McCreight [:mccr8]]

In libnestegg, ne_pool_destroy() calls |h_free(x)|, which just calls |halloc(x, 0)|, which in turn ends up call |realloc(x, 0)|.  One possible implementation of realloc would free x when this happens, but the jemalloc we use instead treats this like |realloc(x, 1)|, so we don't free the block, we just shrink it.

libnestegg is aware of the ambiguity in the implementation and dynamically checks the behavior of realloc, in |_set_allocator|.  If it detects the non-freeing behavior, it uses a little wrapper around realloc, |_realloc|, that just calls free if a length of 0 is passed in.

Unfortunately, as of bug 992955, we no longer call |_set_allocator|.  Instead, we directly set the allocator function ourselves by a call to |nestegg_set_halloc_func|, which bypasses the dynamic check in libnestegg.

I think the easiest fix is to pass in something like libnestegg's _realloc method that simply gives it the expected behavior.

Separately, it would also be nice if libnestegg verified the correct behavior of the function passed in via |nestegg_set_halloc_func|, and at least asserted if it was being violated.  You could do something like assert that |allocator(allocator(0, 1), 0)| is null.  This would leak the object if the assert fails, but for something that is just called once in debug builds, maybe that's ok.  That will have to be done upstream, but maybe we want to have a check for it in our codebase until then.

I believe that LSan can't detect this leak, because ASan uses the realloc behavior expected by libnestegg.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Let libnestegg have a freeing realloc. WIP

This shows what the libnestegg-side part of it might look like, though I'm not planning on landing it here. Without my CountingFreeingRealloc change, the test fails. With it, it passes.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: The realloc for libnestegg should free with size 0.

This is for just fixing the leak.

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8476083 [details] [diff] [review]
The realloc for libnestegg should free with size 0.

Review of attachment 8476083 [details] [diff] [review]:
-----------------------------------------------------------------

try run for a slightly simpler version: https://tbpl.mozilla.org/?tree=Try&rev=bec8eae4106e

Adding checking is going to require work upstream and I'm not sure if I have the energy to push that through.  But we should at least fix the problem locally.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8476083 [details] [diff] [review]
The realloc for libnestegg should free with size 0.

Review of attachment 8476083 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/nsIMemoryReporter.idl
@@ +667,5 @@
>      return pnew;
>    }
>  
> +  static void*
> +  CountingFreeingRealloc(void* p, size_t size)

Hm, should we just be using this as our CountingAllocator realloc replacement generally?

A comment as to why we have this would be appropriate.

::: xpcom/build/nsXPComInit.cpp
@@ +667,5 @@
>  #endif
>  
>  #ifdef MOZ_WEBM
>    // And for libnestegg.
> +  nestegg_set_halloc_func(NesteggReporter::CountingFreeingRealloc);

If we keep separate realloc implementations, a comment here is probably appropriate.

Comment 5

[Matthew Gregan [:kinetik]]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> Adding checking is going to require work upstream and I'm not sure if I have
> the energy to push that through.  But we should at least fix the problem
> locally.

I/we are upstream, and are happy to make whatever changes necessary. :-)

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to Matthew Gregan [:kinetik] from comment #5)
> I/we are upstream, and are happy to make whatever changes necessary. :-)

Thanks.  Where would I file a bug for that?

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd (:froydnj) from comment #4)
> Hm, should we just be using this as our CountingAllocator realloc
> replacement generally?
I'm not a fan of the freeing-realloc behavior, because it results in use-after-frees if misused.

> A comment as to why we have this would be appropriate.
Done.

> If we keep separate realloc implementations, a comment here is probably
> appropriate.
Good point.  Added.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2fb18e695d

Comment 8

[Matthew Gregan [:kinetik]]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> Thanks.  Where would I file a bug for that?

It's hosted on GitHub, issue tracker here: https://github.com/kinetiknz/nestegg/issues

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Matthew Gregan [:kinetik] from comment #8)
> It's hosted on GitHub, issue tracker here:
> https://github.com/kinetiknz/nestegg/issues

Thanks, filed https://github.com/kinetiknz/nestegg/issues/23

Comment 10

[Andrew McCreight [:mccr8]]

Filed bug 1057446 on hooking up CountingAllocatorBase to our leak detection framework, in an attempt to catch leaks like this (and otherwise) in the future.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6c2fb18e695d