Bug 298099 – JSC: Invalid x64 machine code generation due to dangling LOCK prefix

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

# Bug Details
Commit b6667ac3 [1] introduced a simple low-level optimization in the macro assembler backend: if a 32-bit AND operation is needed where one of the operands is the constant -1, then this is a no-op and no code needs to be generated for it:

void and32(TrustedImm32 imm, Address address)
{
    if (imm.m_value == -1)
        return;
    m_assembler.andl_im(imm.m_value, address.offset, address.base);
}

While this optimization is correct in itself, it violates assumptions made elsewhere, in particular that the and32 method [2] will generate an and machine instruction. This becomes problematic for example in atomicAnd32:

void atomicAnd32(TrustedImm32 imm, Address address)
{
    m_assembler.lock();
    and32(imm, address);
}

Here, if the immediate is now -1, the call to and32 will not emit any instructions, causing a dangling lock prefix to be emitted. It seems that this will always lead to one of two outcomes: (1) an illegal instruction error, or (2) a lock prefix being added to a valid instruction. As such, this bug should not have any security implications, but we’re still reporting it as such out of precaution.

[1] https://github.com/WebKit/WebKit/commit/b6667ac3e18f1e2ce20c48e6e2cbbd3610ad8685
[2] https://github.com/WebKit/WebKit/blob/6f1bfd71f727274e800239704dfa477dc1aa2589/Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h#L315
[3] https://github.com/WebKit/WebKit/blob/6f1bfd71f727274e800239704dfa477dc1aa2589/Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h#L4062

# Affected Version(s)
The issue has been successfully reproduced:
* at HEAD (commit 6f1bfd71f727274e800239704dfa477dc1aa2589)

# Reproduction
The crash was reproduced on x64 Linux.

## Test Case
```
let sab = new SharedArrayBuffer(1024);
let int32Arr = new Int32Array(sab);
function buggy_atomic(idx) {
    Atomics.and(int32Arr, idx, -1);
}
for (let i = 0; i < 1000000; ++i) {
    buggy_atomic(i % int32Arr.length);
}
```

## Build Instructions
Obtain the code from https://github.com/WebKit/webkit, then build the jsc-only port using:

    ./Tools/Scripts/build-jsc --jsc-only

## Command

    ./WebKitBuild/bin/jsc crash.js

## Crash Backtrace (from gdb)

Thread 1 "jsc" received signal SIGILL, Illegal instruction.
(gdb) bt
#0  0x00007fffa6a28510 in ?? ()
#1  0x00007ffff676ecbb in llint_call_javascript () from WebKit/WebKitBuild/lib/libJavaScriptCore.so.1
#2  0x00007ffff72301b4 in JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::JSGlobalObject*, JSC::JSObject*) () from WebKit/WebKitBuild/lib/libJavaScriptCore.so.1
#3  0x00007ffff755917a in JSC::evaluate(JSC::JSGlobalObject*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) () from WebKit/WebKitBuild/lib/libJavaScriptCore.so.1
#4  0x000055555557095c in jscmain(int, char**) ()
#5  0x000055555556f76b in main ()
(gdb) x/i $rip
=> 0x7fffa6a28510:      lock mov edi,eax

# Reporter Credit
Google Big Sleep

# Disclosure Policy
Our assessment concluded that this finding has NO security impact. Therefore, we are NOT applying a disclosure deadline to this report.

However, if your internal review indicates a security risk to your users, please email [e-mail] immediately to arrange a standard disclosure deadline.

For more information, visit https://goo.gle/bigsleep

[Radar WebKit Bug Importer]

<rdar://problem/159443883>

[Mark Lam]

Opening this up since this is not a security issue.

[Shu-yu Guo]

Pull request: https://github.com/WebKit/WebKit/pull/50975

[EWS]

Committed 300245@main (e8e09d0cc146): <https://commits.webkit.org/300245@main>

Reviewed commits have been landed. Closing PR #50975 and removing active labels.