Bug 145385 – Integer overflow in XLarge allocation (due to unchecked roundUpToMultipleOf)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Integer overflow in XLarge allocation (due to unchecked roundUpToMultipleOf)

[Geoffrey Garen]

Created attachment 253729 [details]
Patch

[Michael Saboff]

Comment on attachment 253729 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253729&action=review

> Source/bmalloc/bmalloc/Allocator.cpp:112
> +        size = roundUpToMultipleOf<xLargeAlignment>(size);
> +        alignment = std::max(superChunkSize, alignment);

Since superChunkSize(32MB) is larger than xLargeAlignment (page size), Isn't it possible for the size of an aligned extra large allocation to overflow?

[Geoffrey Garen]

> Since superChunkSize(32MB) is larger than xLargeAlignment (page size), Isn't
> it possible for the size of an aligned extra large allocation to overflow?

Which line of code do you think will overflow?

In the XLarge case, we don't do math on xLargeAlignment -- we just pass it through to the kernel. As long as there's no bug in the kernel, we should be OK.

[Andreas Kling]

Comment on attachment 253729 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253729&action=review

r=me

> Source/bmalloc/bmalloc/Allocator.cpp:64
> +        return PerProcess<Heap>::get()->tryAllocateXLarge(lock, superChunkSize, roundUpToMultipleOf<xLargeAlignment>(size));

Can use getFastCase() here.

[Geoffrey Garen]

Committed r184883: <http://trac.webkit.org/changeset/184883>

[Michael Catanzaro]

In Fedora we have 105 reports of this crash still occurring in our latest build with this patch applied (all from 32-bit computers), so something's still wrong here.

[Mario Sanchez Prada]

(In reply to comment #6)
> In Fedora we have 105 reports of this crash still occurring in our latest
> build with this patch applied (all from 32-bit computers), so something's
> still wrong here.

I've seen this since last week in our debian-like Linux system too, right after upgrading to WebKitGTK+ 2.8.3. I've applied the patch here but the crash is not gone yet, still trying to find what's wrong here. Will report back as soon as I have more info.

[Geoffrey Garen]

Which crash?

[Oliver Hunt]

Can someone who's seeing this crash put a stack trace in? Preferably with local var info if you can get it.

[Martin Robinson]

(In reply to comment #8)
> Which crash?

I think Mario is referring to the crash discussed here: https://lists.webkit.org/pipermail/webkit-gtk/2015-June/002381.html

The stack trace for that one is:
(gdb) bt
#0  allocateXLarge () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/bmalloc/bmalloc/Heap.cpp:287
#1  0xb4fc94f5 in allocateXLarge () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/bmalloc/bmalloc/Heap.cpp:293
#2  0xb4fc6ac4 in allocateXLarge () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/bmalloc/bmalloc/Allocator.cpp:227
#3  0xb4fc6b2e in allocateSlowCase () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/bmalloc/bmalloc/Allocator.cpp:245
#4  0xb4f95de2 in allocate () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/bmalloc/bmalloc/Allocator.h:86
#5  allocate () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/bmalloc/bmalloc/Cache.h:79
#6  malloc () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/bmalloc/bmalloc/bmalloc.h:43
#7  fastMalloc () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/FastMalloc.cpp:270
#8  0xb5592815 in allocateBuffer () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/Vector.h:269
#9  reserveCapacity () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/Vector.h:1090
#10 expandCapacity () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/Vector.h:951
#11 0xb5f766e1 in expandCapacity () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/Vector.h:958
#12 appendSlowCase<WebCore::FloatRect&> () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/Vector.h:1219
#13 append<WebCore::FloatRect&> () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/Vector.h:1210
#14 createOrDestroyTilesIfNeeded () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:89
#15 0xb5f77001 in updateContents () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:148
#16 0xb64aefb3 in updateBackingStoreIfNeeded () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:549
#17 0xb64af095 in updateBackingStoreIncludingSubLayers () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:519
#18 0xb64af0ed in updateBackingStoreIncludingSubLayers () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:526
#19 0xb64af0ed in updateBackingStoreIncludingSubLayers () at
/home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:526
[...]

[Carlos Alberto Lopez Perez]

The report on RH bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=1225733
https://retrace.fedoraproject.org/faf/reports/605326/

[Michael Catanzaro]

(In reply to comment #8)
> Which crash?

Eh, the Fedora crash report was thought to be caused by this bug, but I see there is no report of any crash in this bug. We could create a separate WebKit bug report for this if you want.

We usually always have a full backtrace with local variable info (for whichever variables survived optimization) but this time around we don't, due to a flaw in the Fedora crash tracker. Maybe Mario can provide that, since he can reproduce it reliably.

[Geoffrey Garen]

That crash is not related to integer overflow.

[Mario Sanchez Prada]

(In reply to comment #12)
> (In reply to comment #8)
> > Which crash?
> 
Sorry about that, I've been trying to wrap my head today around so many things that I reply too quickly, without actually realizing that this bug was not about any crash :/

In any case, I was referring to the crash that Martin kindly pasted here, which I've been reliably reproducing 100% of the times in my Ubuntu-like 32-bit box whenever I run an application that embeds and uses WebKitGTK+ through GObject Introspection (more details here: https://lists.webkit.org/pipermail/webkit-gtk/2015-June/002381.html)

> Eh, the Fedora crash report was thought to be caused by this bug, but I see
> there is no report of any crash in this bug. We could create a separate
> WebKit bug report for this if you want.
> 

Same here. I also thought that this crash could have be caused by the same issue, and therefore I also associated it somehow with this patch. But perhaps is not related, as Geoffrey mentioned in his last comment.

I'll file a new bug then, now that it seems pretty obvious that is not only happening to me (the fedora backtrace is nearly identical)...

> We usually always have a full backtrace with local variable info (for
> whichever variables survived optimization) but this time around we don't,
> due to a flaw in the Fedora crash tracker. Maybe Mario can provide that,
> since he can reproduce it reliably.

Not this time, sorry. The backtrace you are seeing now is the best that I could get so far, which is taken out of a DEBUG build with -O0 and -g1 passed as CFLAGS. I'd love to have passed -g2 or -g3, so that I can get the actual values in the backtrace, but I haven't found a way to do it because, being a 32-bit build, whenever I get to the linking stage the building process dies trying to link 2.4GB of shared objects.

If you, or anyone else, knows a way I can still get a more meaningful backtrace for this 32-bit build, I'm more than extremely happy to try that out, so please speak up :)

(In reply to comment #13)
> That crash is not related to integer overflow.

Thanks for confirming that. Any idea on what this could be a about then?

[Mario Sanchez Prada]

(In reply to comment #14)
> [...]
> I'll file a new bug then, now that it seems pretty obvious that is not only
> happening to me (the fedora backtrace is nearly identical)...

New bug reported: https://bugs.webkit.org/show_bug.cgi?id=146440