Closed Bug 1782124 Opened 3 years ago Closed 3 years ago

src/js/src/jit/x86-shared/Assembler-x86-shared.h:4795:5: runtime error: store to misaligned address 0x3a4d55f6288d for type 'int32_t' (aka 'int'), which requires 4 byte alignment

Categories

(Core :: JavaScript Engine: JIT, defect, P3)

defect

Tracking

()

RESOLVED FIXED
105 Branch
Tracking Status
firefox105 --- fixed

People

(Reporter: tsmith, Assigned: nbp)

References

(Blocks 2 open bugs)

Details

(Keywords: csectype-undefined)

Attachments

(1 file)

This was found by enabling the alignment check in UBSan and launching the browser. This type of issue can create inconsistencies across platforms, architectures and optimization levels.

Found with m-c 20220728-c067b27a1842

To enable this check add the following to your mozconfig:

ac_add_options --enable-undefined-sanitizer="alignment"
src/js/src/jit/x86-shared/Assembler-x86-shared.h:4795:5: runtime error: store to misaligned address 0x3a4d55f6288d for type 'int32_t' (aka 'int'), which requires 4 byte alignment
0x3a4d55f6288d: note: pointer points here
 0e 48 8d 1d 00 00 00  00 ff 24 cb 49 bb 00 00  00 00 00 80 f9 ff 41 53  49 83 c6 01 0f 1f 44 00  00
             ^ 
    #0 0x7f01d86debb5 in js::jit::AssemblerX86Shared::PatchWrite_Imm32(js::jit::CodeLocationLabel, js::jit::Imm32) src/js/src/jit/x86-shared/Assembler-x86-shared.h:4795:38
    #1 0x7f01d86de7b9 in js::jit::MacroAssembler::patchNearAddressMove(js::jit::CodeLocationLabel, js::jit::CodeLocationLabel) src/js/src/jit/x64/MacroAssembler-x64.cpp:1599:3
    #2 0x7f01d7c3f7f3 in js::jit::BaselineInterpreterGenerator::generate(js::jit::BaselineInterpreter&) src/js/src/jit/BaselineCodeGen.cpp:6646:7
    #3 0x7f01d7cd7578 in js::jit::GenerateBaselineInterpreter(JSContext*, js::jit::BaselineInterpreter&) src/js/src/jit/BaselineJIT.cpp:952:22
    #4 0x7f01d93ca14e in js::jit::JitRuntime::initialize(JSContext*) src/js/src/jit/Ion.cpp:123:8
    #5 0x7f01d5c4ada1 in JSRuntime::createJitRuntime(JSContext*) src/js/src/vm/Realm.cpp:138:21
    #6 0x7f01d546f10d in JS::InitSelfHostedCode(JSContext*, mozilla::Span<unsigned char const, 18446744073709551615ul>, bool (*)(JSContext*, mozilla::Span<unsigned char const, 18446744073709551615ul>)) src/js/src/vm/Initialization.cpp:237:14
    #7 0x7f01a07673c5 in XPCJSContext::Initialize() src/js/xpconnect/src/XPCJSContext.cpp:1369:8
    #8 0x7f01a0769512 in XPCJSContext::NewXPCJSContext() src/js/xpconnect/src/XPCJSContext.cpp:1405:23
    #9 0x7f01a08de749 in nsXPConnect::InitJSContext() src/js/xpconnect/src/nsXPConnect.cpp:88:25
    #10 0x7f01a08dea40 in xpc::InitializeJSContext() src/js/xpconnect/src/nsXPConnect.cpp:103:35
    #11 0x7f01d43c3c3f in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:5415:7
    #12 0x7f01d43c7ede in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:5894:8
    #13 0x7f01d43c87ea in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:5962:21
    #14 0x7f01d4408fc0 in mozilla::BootstrapImpl::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/Bootstrap.cpp:45:12
    #15 0x55ce43bb7f44 in do_main(int, char**, char**) src/browser/app/nsBrowserApp.cpp:227:22
    #16 0x55ce43bb6826 in main src/browser/app/nsBrowserApp.cpp:414:16
    #17 0x7f020dbe1c86 in __libc_start_main /build/glibc-CVJwZb/glibc-2.27/csu/../csu/libc-start.c:310
    #18 0x55ce43af5ec8 in _start (src/objdir-ff-ubsan/dist/bin/firefox+0x31aec8) (BuildId: 9a24bec1c7ae3ec1a6e75a9712b8aed683e61a7e)

Well …, I completely disagree with this UBSan report.

Is there a way to add this one to an allow-list?

The alternative would be add a new type named unaligned_int32_t which behaves like an int32_t and can be copied from/to an int32_t such that UBSan does not complain about it.

Blocks: sm-security
Severity: -- → S4
Priority: -- → P3

(In reply to Nicolas B. Pierron [:nbp] from comment #1)

Well …, I completely disagree with this UBSan report.

An acceptable way to handle this would be to use memcpy.

Perhaps some thing like (untested):

memcpy(*((int32_t*)dataLabel.raw() - 1), &toWrite.value, sizeof(int32_t));

The call to memcpy should be optimized out and this not undefined behavior (in C).

Is there a way to add this one to an allow-list?

I can add it if needed, but it's much nicer to update the code than maintain a suppression list if possible. I'd be happy to test any patches if needed.

Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED

This patch changes the x86/x64 backend to:

  • avoid unaligned write of int32_t, unaligned read of uintptr_t values.
  • replace typed pointer arithmetic by byte offsets only.
  • add comments with opcode when patching assembly.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 105 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: