566017 - JEMalloc's posix_memalign() asserts that size!=0

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

All's in the title. The posix_memalign() and memalign() functions provided by JEMalloc are asserting that size!=0. At least here on Linux; looking at the source code this seems to be the case on all platforms except perhaps Darwin.

More specifically, these functions are calling an internal function arena_malloc() that is making that assertion at line 3792 of jemalloc.c.

They shouldn't be making that assertion:
* malloc() itself, including JEMalloc's implementation, is not asserting size!=0.
* The GNU man page for posix_memalign says that size==0 is allowed.
* Libraries that we use are potentially calling posix_memalign with size==0. For example OSMesa (off-screen Mesa OpenGL library), that we optionally use in WebGL, does that when one calls glTexImage2D with 0x0 texture size, forcing us to use a small work-around to avoid crashing, see bug 565998.

The proper fix probably consists in copying what malloc() does in jemalloc.c.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Let memalign accept the 0 size

This patch lets memalign() accept the 0 size. It just copies what is being done in malloc(), so there's no original code here.

This also fixes the posix_memalign() function, since it is just calling memalign() and the assert occurs further down (in the ipalloc() call).

This patch means that we don't crash anymore in WebGL when using a software renderer like OSMesa and creating a 0-sized texture. Or at least that we don't need a work-around anymore. See 565998.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Note: I don't know whom to ask for review for this patch? I couldn't find a bugmail id for Jason Evans.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Note: if you want to see where this if() block was copied from, it's lines 5777-5788, in the malloc() function. Also note that realloc() too already has a similar block.

Comment 4

[Jason Evans]

Comment on attachment 445542 [details] [diff] [review]
Let memalign accept the 0 size

Looks good, though you can leave the comment out if you want.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Cool Jason, just one question. I put that in memalign() because posix_memalign() is currently calling it anyway, but what if in the future someone eventually makes posix_memalign() no longer call memalign()? Should I at least put a comment in posix_memalign to remind future coders about this?

Comment 6

[Jason Evans]

That sounds like a good idea to me (/* 0-->1 size promotion already done. */ or somesuch).

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Updated patch (same with improved comment to reflect discussion)

Attached updated patch, with just the comment changed.

I don't have write access to mozilla-central. Can someone commit that for me?

I also don't know if I should ask for review again. Feel free :)

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 445689 [details] [diff] [review]
Updated patch (same with improved comment to reflect discussion)

Carrying forward r+ from Jason.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/f0e2872e3d15