Closed Bug 1574415 Opened 3 months ago Closed Last month

Differential Testing: Different output message involving TypedArrays

Categories

(Core :: JavaScript Engine: JIT, defect, P1, critical)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed

People

(Reporter: gkw, Assigned: anba)

References

(Blocks 2 open bugs, Regression)

Details

(Keywords: regression, testcase)

Attachments

(14 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
for (let x of [0, allocationMarker()]) {
    try {
        (new Int8Array)[1] = x;
    } catch (e) {
        print(e);
    }
}
$ ./js-dbg-64-dm-linux-x86_64-d887276421d3 --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js 
$
$ ./js-dbg-64-dm-linux-x86_64-d887276421d3 --fuzzing-safe --no-threads --no-baseline --no-ion testcase.js 
TypeError: can't convert ({}) to number

Tested this on m-c rev d887276421d3.

My configure flags are:

AR=ar sh ./configure --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/3030106b14f7
user: André Bargull
date: Fri Jun 29 13:46:04 2018 -0700
summary: Bug 1471788: Apply ToNumber conversion even when the typed array write is out-of-bounds. r=evilpie

Setting s-s as a start as TypedArrays are involved. Andre, is bug 1471788 a likely regressor?

Flags: needinfo?(andrebargull)

Test case without allocationMarker:

for (let x of [0, Object.create(null)]) {
    try {
        (new Int8Array)[1] = x;
    } catch (e) {
        print(e);
    }
}

It looks like Baseline (and maybe Ion, still need to check the relevant code) is missing the ToNumber conversion for out-of-bounds writes (which was added in bug 1471788). I don't think this is a security issue, but merely a spec compliance bug, but let me first dig into the code before potentially opening the report.

Flags: needinfo?(andrebargull)
Summary: Differential Testing: Different output message involving allocationMarker and TypedArrays → Differential Testing: Different output message involving TypedArrays

This ensures CacheIR doesn't completely ignore out-of-bounds writes to
TypedArray objects and also enables to use additional scratch registers for
bounds-checking.

StoreToTypedArray is now only used for TypedObjects and always passes
Address for the dest argument.

Depends on D42810

Priority: -- → P1

(In reply to André Bargull [:anba] from comment #1)

I don't think this is a security issue, but merely a spec compliance bug, but let me first dig into the code before potentially opening the report.

"out of bound writes" sounds like a security problem, or is this more about how/whether we generate errors when that attempt is detected? Now that you have a patch did this turn out to be a security problem or can we unhide it?

Assignee: nobody → andrebargull
Flags: needinfo?(andrebargull)

(In reply to Daniel Veditz [:dveditz] from comment #4)

"out of bound writes" sounds like a security problem, or is this more about how/whether we generate errors when that attempt is detected?

The latter. We had something like:

if (index < length) {
  if (value is not a Number) {
    goto failure;
  }
  typedArray[index] = value;
}

but per spec we should have done:

if (value is not a Number) {
  goto failure;
}
if (index < length) {
  typedArray[index] = value;
}

Now that you have a patch did this turn out to be a security problem or can we unhide it?

We can unhide it.

Flags: needinfo?(andrebargull)

Unhiding as per comment 5.

Group: javascript-core-security

Removing "regression" keyword, because it's not a regression.

Keywords: regression

André are you still looking into fixing this issue? Do you need more clarification on Jan's comments?

Flags: needinfo?(andrebargull)

Jan's review comments/suggestions led to a few additional changes. More reviews are upcoming! :-)

Flags: needinfo?(andrebargull)

Add a RAII class to spill and restore Float0 when used as a scratch register
when generating Ion CacheIR assembly. It's still possible to generate incorrect
code which doesn't properly restore Float0, for example through jump
instructions, but the RAII class should at least prevent bugs like in
CacheIRCompiler::emitLoadDoubleTruthyResult where Float0 wasn't restored
for the truthy case.

Depends on D42811

IonCacheIRCompiler used MacroAssembler::truncateConstantOrRegisterToInt32 resp.
clampConstantOrRegisterToUint8, which allowed to handle more types than just
int32 and double, but since SetPropIRGenerator::tryAttachSetTypedElement only
allows number inputs, this code was probably never taken in practice. Therefore
the CacheIRCompiler implementation for both methods only handles int32 and
double inputs.

As an optimisation to generate better assembly and to reduce register pressure,
both methods have special-cases for known int32 inputs and are using
ScratchTagScope to avoid splitting the tag two times.

Part 7 will move the duplicated code into a shared helper function.

Depends on D47751

This avoids emitting movapd %xmm0, %xmm0 for clamp-uint8 code.

Depends on D47752

Similar to emitGuardToInt32ModUint32 and emitGuardToUint8Clamped, ScratchTagScope
can be used in emitGuardToInt32Index to avoid splitting the tag two times.

The next part will move this code into a shared helper function.

Depends on D47753

The SFINAE code can be removed when C++17's if-constexpr is available.

Depends on D47754

These methods are no longer necessary after the changes from part 4.

Depends on D47755

Changes LSetPropertyCache to use Float0 as a fixed temporary register,
which makes it easier to share the implementation between Baseline and Ion.
Additionally this fixes a TypedObject bug where Float0 was clobbered, see
the regression test in part 11.

Also adds support for Constant, PayloadReg, and PayloadStack operand
locations to CacheRegisterAllocator::ensureDoubleRegister, which are now
needed because the right-hand side in TypedArray assignments can be constants
resp. values in payload registers or on the stack.

Depends on D47756

These methods are no longer necessary after the changes from part 9.

Depends on D47757

Regression test for the fixed TypedObject bug mentioned in part 9.

Depends on D47758

After adding emitAddressFromStubField(), the emitStoreTypedObjectScalarProperty
implementations in the Baseline and Ion CacheIR compiler are exactly equal to
each other and hence can be moved into CacheIRCompiler.

Depends on D47760

ta[0] = 0; emits the following assembly for Ion CacheIR without this patch:

[Codegen] emitStoreTypedElement
[Codegen] push       %rbx
[Codegen] push       %rbp
[Codegen] movl       0x28(%rax), %ebx
[Codegen] xorl       %r11d, %r11d
[Codegen] cmpl       %ebx, %edx
[Codegen] jae        .Lfrom47
[Codegen] cmovae     %r11d, %edx
[Codegen] movq       0x38(%rax), %rbx
[Codegen] movl       %ecx, 0x0(%rbx,%rdx,4)

When not allocating the unused scratch register for Spectre bounds checks, this
assembly is emitted:

[Codegen] emitStoreTypedElement
[Codegen] push       %rdx
[Codegen] movl       0x28(%rax), %edx
[Codegen] xorl       %r11d, %r11d
[Codegen] cmpl       %edx, %ebx
[Codegen] jae        .Lfrom46
[Codegen] cmovae     %r11d, %ebx
[Codegen] movq       0x38(%rax), %rdx
[Codegen] movl       %ecx, 0x0(%rdx,%rbx,4)

Which shows %rbp is no longer spilled on the stack, resulting in a minor
performance improvement (~3%) on µ-benchmarks and also avoiding a performance
regression (performance is now again on par with the state before this patch
series). For comparison this assembly was generated before this patch series:

[Codegen] emitStoreTypedElement
[Codegen] push       %rbx
[Codegen] movl       0x28(%rax), %ecx
[Codegen] xorl       %r11d, %r11d
[Codegen] cmpl       %ecx, %edx
[Codegen] jae        .Lfrom44
[Codegen] cmovae     %r11d, %edx
[Codegen] movq       0x38(%rax), %rcx
[Codegen] xorl       %ebx, %ebx
[Codegen] movl       %ebx, 0x0(%rcx,%rdx,4)

Depends on D47761

This is an epic patch stack, thanks so much for all the clean up work!

Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/d2008fdba4af
Part 1: Move argument conversion for StoreTypedElement to happen before range checks. r=jandem
https://hg.mozilla.org/integration/autoland/rev/6067d1828df8
Part 2: Remove no longer used template argument and rename 'StoreToTypedArray'. r=jandem
https://hg.mozilla.org/integration/autoland/rev/3f73e398ca61
Part 3: Add AutoScratchFloatRegister. r=jandem
https://hg.mozilla.org/integration/autoland/rev/ef1f7213e654
Part 4: Move emitGuardToInt32ModUint32 and emitGuardToUint8Clamped into CacheIRCompiler. r=jandem
https://hg.mozilla.org/integration/autoland/rev/f3b48a00be45
Part 5: Avoid unnecessary move when source and temporary registers are equal. r=jandem
https://hg.mozilla.org/integration/autoland/rev/1cc3c2eee37e
Part 6: Use ScratchTagScope for CacheIRCompiler::emitGuardToInt32Index. r=jandem
https://hg.mozilla.org/integration/autoland/rev/c24cec7a2424
Part 7: Add helper function when guarding int32-or-double to avoid code duplication. r=jandem
https://hg.mozilla.org/integration/autoland/rev/6eead2ab3379
Part 8: Remove no longer used MacroAssembler methods. r=jandem
https://hg.mozilla.org/integration/autoland/rev/3601bf791733
Part 9: Move emitStoreTypedElement into CacheIRCompiler. r=jandem
https://hg.mozilla.org/integration/autoland/rev/42469f1d0491
Part 10: Remove no longer used MacroAssembler methods. r=jandem
https://hg.mozilla.org/integration/autoland/rev/1bd9ae4caa0b
Part 11: Add regression test for StoreTypedObjectScalarProperty clobbered register bug. r=jandem
https://hg.mozilla.org/integration/autoland/rev/218a81625331
Part 12: Remove StoreToTypedObject and instead use new guard ops for StoreTypedObjectScalarProperty. r=jandem
https://hg.mozilla.org/integration/autoland/rev/3dc778bc0ce0
Part 13: Move emitStoreTypedObjectScalarProperty into CacheIRCompiler. r=jandem
https://hg.mozilla.org/integration/autoland/rev/144ebbca6844
Part 14: Don't allocate Spectre bounds check scratch register when it's unused. r=jandem

Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.