1487815 - Likely UB in LifoAlloc::newChunkWithCapacity

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Jason Orendorff [:jorendorff]]

https://searchfox.org/mozilla-central/rev/05d91d3e02a0780f44599371005591d7988e2809/js/src/ds/LifoAlloc.cpp#181-182

    // Compute the size which should be requested in order to be able to fit |n|
    // bytes in a newly allocated chunk, or default to |defaultChunkSize_|.
    uint8_t* u8begin = nullptr;
    uint8_t* u8end = u8begin + detail::BumpChunkReservedSpace;
    u8end = detail::BumpChunk::nextAllocEnd(detail::BumpChunk::nextAllocBase(u8end), n);
    size_t allocSizeWithCanaries = u8end - u8begin;

Maybe compilers have to accommodate this trick, since so much old code uses it for slight variations on offsetof(). But I think the standard says it's UB.

ni?Waldo for a ruling.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Compute LifoAlloc chunk-size in a manner that doesn't invoke UB

C++ does not appear to define arithmetic on pointers that do not point to valid objects (or arrays of them), and nullptr as a null pointer constant does not.  So yes, this is UB.  Gotta do math on sizes.

Wish this were a smaller line-change count, but I'm not much sure it can be helped.

Comment 2

[Ted Campbell [:tcampbell]]

Comment on attachment 9007126 [details] [diff] [review]
Compute LifoAlloc chunk-size in a manner that doesn't invoke UB

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

Seems cleaner.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb16d5fe593
Compute LifoAlloc chunk-size in a manner that doesn't invoke UB.  r=tcampbell

Comment 4

[Andreea Pavel [:apavel]]

Backed out for build bustages on a closed tree

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4bb16d5fe59315ab0e7f31e2eff6f6a162002cba

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=198163167&repo=mozilla-inbound&lineNumber=5059

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/94a74f38a657f47ca054f7feaa8f96628bfc9065

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55690779a96d
Compute LifoAlloc chunk-size in a manner that doesn't invoke UB.  r=tcampbell

Comment 6

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/55690779a96d

Comment 7

[Nicolas B. Pierron [:nbp]]

Honestly, I find the original code cleaner than the new one added here to work-around some Undefined Behaviour. The code use before had the advantage of sharing the same logic as the one which are making the allocation.

Comment 8

[Jeff Walden [:Waldo]]

The old way is simply impermissible in C++, so it is not feasible to return to it.

The best I can say for this, in terms of simplifying it if we wanted to do so, would be to encapsulate all size-computation and offsets into a single location somewhere, then have both *this* code, and nextAllocBase/nextAllocEnd, both use those constants.  I somewhat debated doing that, but it seemed slightly excessive.  This code doesn't change all that often; we are not encapsulating *that* much complexity.  But if you feel strongly about it, feel free to improve things in this manner.