1798883 - Assertion failure: result in FormatBuffer.h

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect, P2)

Description

[Samuel Groß]

The following sample triggers an assertion failure in debug builds of Spidermonkey from latest HEAD:

function main() {
for (let v2 = 1; v2 < 100; v2++) {
    try {
        const v3 = 54321;
        for (let v8 = 0; v8 < 100; v8++) {
            let v12 = 15;
            const v13 = v12--;
            const v14 = -1094231111 >> v12;
            const v15 = "oUtXlGoXpU".constructor;
            const v16 = v15.fromCharCode(v14);
            const v17 = [v16];
            const v18 = v17.unshift(Date);
            const v19 = v15(v17);
            const v20 = v19.normalize();
            const v21 = v8 >>> v8;
            const v22 = v21 >> 100;
            try {
                const v24 = this.oomAtAllocation(100,v22);
                const v26 = "0NnanVrDTQ".call();
            } catch(v27) {
            } finally {
            }
        }
        const v30 = this.oomAtAllocation(v2);
    } catch(v33) {
    }
}
gc();
}
main();
// CRASH INFO
// ==========
// TERMSIG: 11
// STDERR:
// Assertion failure: result, at /home/builder/firefox/js/src/builtin/intl/FormatBuffer.h:77
// #01: ???[./spidermonkey/js +0x1d7f884]
// #02: ???[./spidermonkey/js +0x1d7f3d3]
// #03: ???[./spidermonkey/js +0x1d7d02b]
// #04: ??? (???:???)

Here is the backtrace from gdb:

#0  0x0000555557b02f29 in js::intl::FormatBuffer<char16_t, 32ul, js::TempAllocPolicy>::written (this=0x7fffffff9840, amount=40) at js/src/builtin/intl/FormatBuffer.h:77
#1  0x0000555557aef388 in mozilla::intl::FillBufferWithICUCall<mozilla::intl::String::Normalize<js::intl::FormatBuffer<char16_t, 32ul, js::TempAllocPolicy> >(mozilla::intl::String::NormalizationForm, mozilla::Span<char16_t const, 18446744073709551615ul>, js::intl::FormatBuffer<char16_t, 32ul, js::TempAllocPolicy>&)::{lambda(char16_t*, int, UErrorCode*)#1}, js::intl::FormatBuffer<char16_t, 32ul, js::TempAllocPolicy> >(js::intl::FormatBuffer<char16_t, 32ul, js::TempAllocPolicy>&, mozilla::intl::String::Normalize<js::intl::FormatBuffer<char16_t, 32ul, js::TempAllocPolicy> >(mozilla::intl::String::NormalizationForm, mozilla::Span<char16_t const, 18446744073709551615ul>, js::intl::FormatBuffer<char16_t, 32ul, js::TempAllocPolicy>&)::{lambda(char16_t*, int, UErrorCode*)#1} const&) (buffer=..., strFn=...) at obj-debug/dist/include/mozilla/intl/ICU4CGlue.h:184
#2  0x0000555557b02ab2 in mozilla::intl::String::Normalize<js::intl::FormatBuffer<char16_t, 32ul, js::TempAllocPolicy> > (aForm=mozilla::intl::String::NormalizationForm::NFC, aString=..., aBuffer=...) at obj-debug/dist/include/mozilla/intl/String.h:157
#3  0x0000555557aee09d in str_normalize (cx=0x7ffff6622e00, argc=0, vp=0x7fffffff9978) at js/src/builtin/String.cpp:1546
#4  0x000017d2e5c7289f in ?? ()
#5  0x0000000000000000 in ?? ()

I suspect the issue here is an OOM during string.normalize, which causes the FormatBuffer.resizeUninitialized to fail. The result seems to be that the string returned from normalize is shorter than it should be (for me its length is 38 instead of 40 when the resize fails). I don't think this has any security impact, but I'm still filing it as a security issue as a precaution.

Comment 1

[Steven DeTar [:sdetar]]

Moving this to the "JavaScript: Internationalization API" component

Comment 2

[Greg Tatum [:gregtatum]]

This seems like it should be fixed, as it sounds like a correctness issue. We surface the OOM errors, and shouldn't fail on an assert like that. This seems like a logic error in the FormatBuffer. This class makes me nervous in general, since it's doing raw memory management.

Comment 3

[André Bargull [:anba]]

This issue doesn't seem security-sensitive, because it can only happen when OOMs are simulated:

FormatBuffer::written() calls Vector::resizeUninitialized(), which in turn calls Vector::growByUninitialized(). In Vector::growByUninitialized() we execute maybeCheckSimulatedOOM() which leads to returning false, which then triggers the assertion in FormatBuffer::written(). The (aIncr > mTail.mCapacity - mLength) condition in Vector::growByUninitialized() is never true, because we previously already reserved enough space, cf. MOZ_ASSERT(amount <= buffer_.capacity()) in FormatBuffer::written().

To fix this bug it's probably enough to use one of the "infallible" Vector methods which don't use simulated OOMs.

Comment 4

[Dan Minor [:dminor]]

What seems to be happening is that the length we get from calling the function in FillBufferWithICUCall at [1] does not match what was previously reserved at [2]. In the test case here, it's one character longer that we reserved, maybe because of a null terminator. We pass the capacity into the ICU4C function to determine if there is enough space, not the reserved amount, so we don't get a U_BUFFER_OVERFLOW_ERROR and try to reserve more space in this case. This is fine, we can use up to capacity without needing to allocate memory.

Like Anba said, this is only a problem when OOMs are simulated, because maybeCheckSimulatedOOM() considers the amount that was previously reserved, not the overall capacity of the vector when deciding whether to fail.

[1] https://searchfox.org/mozilla-central/rev/eddb810ffd5499f0984123fe4bfea6064ebad3c8/intl/components/src/ICU4CGlue.h#168
[2] https://searchfox.org/mozilla-central/rev/eddb810ffd5499f0984123fe4bfea6064ebad3c8/intl/components/src/String.h#146

Comment 5

[Dan Minor [:dminor]]

Attachment: Bug 1798883 - Reserve capacity in Buffer::reserve; r=anba!

Although not documented as such, the unorm2_normalizeSecondAndAppend appends a
null terminator to the output string, so we need to reserve an extra character
for that. This causes problems with debug builds, because we do not reserve
enough characters and so will fail assertions, but is not a problem in
non-debug builds because we are already properly ensuring that we have
sufficient capacity in the output buffer.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17d495325e97
Reserve capacity in Buffer::reserve; r=anba

Comment 7

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/17d495325e97