Closed Bug 1413049 Opened 2 years ago Closed 2 years ago

UBSan: store to misaligned address for type 'uintptr_t' (aka 'unsigned long')

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: tsmith, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-undefined)

Attachments

(2 files)

No description provided.
This is triggered on start up when Firefox is built with "-fsanitize=alignment"

/mozilla-central/js/src/jit/x86-shared/Assembler-x86-shared.h:3648:9: runtime error: store to misaligned address 0x22698fe3029a for type 'uintptr_t' (aka 'unsigned long'), which requires 8 byte alignment
0x22698fe3029a: note: pointer points here
 62 70  49 bb ff ff ff ff ff ff  ff ff 41 53 6a 00 85 c0  0f 84 34 00 00 00 83 f8  01 0f 84 03 01 00
              ^ 
    #0 0x7fb0e70bba97 in PatchDataWithValueCheck /mozilla-central/js/src/jit/x86-shared/Assembler-x86-shared.h:3648:14
    #1 0x7fb0e70bba97 in PatchDataWithValueCheck /mozilla-central/js/src/jit/x86-shared/Assembler-x86-shared.h:3651
    #2 0x7fb0e70bba97 in linkSelfReference /mozilla-central/js/src/jit/MacroAssembler.cpp:2891
    #3 0x7fb0e70bba97 in js::jit::MacroAssembler::link(js::jit::JitCode*) /mozilla-central/js/src/jit/MacroAssembler.cpp:2364
    #4 0x7fb0e7027a8e in js::jit::JitCode* js::jit::Linker::newCode<(js::AllowGC)0>(JSContext*, js::jit::CodeKind, bool) /mozilla-central/js/src/jit/Linker.cpp:54:10
    #5 0x7fb0e737303e in js::jit::JitRuntime::generateBailoutTailStub(JSContext*) /mozilla-central/js/src/jit/x64/Trampoline-x64.cpp:990:28
    #6 0x7fb0e6d37734 in js::jit::JitRuntime::initialize(JSContext*, js::AutoLockForExclusiveAccess&) /mozilla-central/js/src/jit/Ion.cpp:248:20
    #7 0x7fb0e74b47a8 in JSRuntime::createJitRuntime(JSContext*) /mozilla-central/js/src/jscompartment.cpp:198:23
    #8 0x7fb0e8102abc in getJitRuntime /mozilla-central/js/src/vm/Runtime.h:681:50
    #9 0x7fb0e8102abc in JS::Zone::createJitZone(JSContext*) /mozilla-central/js/src/gc/Zone.cpp:277
    #10 0x7fb0e74b4abb in getJitZone /mozilla-central/js/src/gc/Zone.h:293:80
    #11 0x7fb0e74b4abb in JSCompartment::ensureJitCompartmentExists(JSContext*) /mozilla-central/js/src/jscompartment.cpp:215
    #12 0x7fb0e6b72fed in CanEnterBaselineJIT(JSContext*, JS::Handle<JSScript*>, js::InterpreterFrame*) /mozilla-central/js/src/jit/BaselineJIT.cpp:295:29
    #13 0x7fb0e6b734e6 in js::jit::CanEnterBaselineMethod(JSContext*, js::RunState&) /mozilla-central/js/src/jit/BaselineJIT.cpp:358:12
    #14 0x7fb0e6ecd2f5 in js::jit::MaybeEnterJit(JSContext*, js::RunState&) /mozilla-central/js/src/jit/Jit.cpp:158:40
    #15 0x7fb0e68ad2de in js::RunScript(JSContext*, js::RunState&) /mozilla-central/js/src/vm/Interpreter.cpp:407:34
    #16 0x7fb0e68e665b in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /mozilla-central/js/src/vm/Interpreter.cpp:705:15
    #17 0x7fb0e68e6d2c in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /mozilla-central/js/src/vm/Interpreter.cpp:737:12
    #18 0x7fb0e74888bb in Evaluate(JSContext*, js::ScopeKind, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) /mozilla-central/js/src/jsapi.cpp:4792:19
    #19 0x7fb0e74882d8 in JS::Evaluate(JSContext*, JS::ReadOnlyCompileOptions const&, char const*, unsigned long, JS::MutableHandle<JS::Value>) /mozilla-central/js/src/jsapi.cpp:4832:15
    #20 0x7fb0e7b2071a in JSRuntime::initSelfHosting(JSContext*) /mozilla-central/js/src/vm/SelfHosting.cpp:2906:10
    #21 0x7fb0e74557aa in JS::InitSelfHostedCode(JSContext*) /mozilla-central/js/src/jsapi.cpp:660:14
    #22 0x7fb0dabc06f3 in nsXPConnect::InitStatics() /mozilla-central/js/xpconnect/src/nsXPConnect.cpp:135:10
    #23 0x7fb0dab113b8 in xpcModuleCtor() /mozilla-central/js/xpconnect/src/XPCModule.cpp:13:5
    #24 0x7fb0e2632997 in Initialize() /mozilla-central/layout/build/nsLayoutModule.cpp:316:8
    #25 0x7fb0d85c7d32 in nsComponentManagerImpl::KnownModule::Load() /mozilla-central/xpcom/components/nsComponentManager.cpp:763:21
    #26 0x7fb0d85c99a1 in nsFactoryEntry::GetFactory() /mozilla-central/xpcom/components/nsComponentManager.cpp:1785:19
    #27 0x7fb0d85cb4ba in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /mozilla-central/xpcom/components/nsComponentManager.cpp:1083:41
    #28 0x7fb0d85c09f2 in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) /mozilla-central/xpcom/components/nsComponentManager.cpp:1446:10
    #29 0x7fb0d85d3368 in CallGetService /mozilla-central/xpcom/components/nsComponentManagerUtils.cpp:67:43
    #30 0x7fb0d85d3368 in nsGetServiceByContractID::operator()(nsID const&, void**) const /mozilla-central/xpcom/components/nsComponentManagerUtils.cpp:280
    #31 0x7fb0d843a9ad in nsCOMPtr_base::assign_from_gs_contractid(nsGetServiceByContractID, nsID const&) /mozilla-central/xpcom/base/nsCOMPtr.cpp:95:7
    #32 0x7fb0d86c2c7d in nsCOMPtr /mozilla-central/objdir-ff-ubsan/dist/include/nsCOMPtr.h:928:5
    #33 0x7fb0d86c2c7d in NS_InitXPCOM2 /mozilla-central/xpcom/build/XPCOMInit.cpp:709
    #34 0x7fb0e6559dca in ScopedXPCOMStartup::Initialize() /mozilla-central/toolkit/xre/nsAppRunner.cpp:1573:8
    #35 0x7fb0e656e0ec in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /mozilla-central/toolkit/xre/nsAppRunner.cpp:4844:22
    #36 0x7fb0e656f863 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /mozilla-central/toolkit/xre/nsAppRunner.cpp:4943:21
    #37 0x516db8 in do_main /mozilla-central/browser/app/nsBrowserApp.cpp:231:22
    #38 0x516db8 in main /mozilla-central/browser/app/nsBrowserApp.cpp:304
    #39 0x7fb0fe6fe1c0 in __libc_start_main /build/glibc-CxtIbX/glibc-2.26/csu/../csu/libc-start.c:308
    #40 0x41f549 in _start (/mozilla-central/objdir-ff-ubsan/dist/bin/firefox+0x41f549)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /mozilla-central/js/src/jit/x86-shared/Assembler-x86-shared.h:3648:9
Summary: UBSanstore to misaligned address for type 'uintptr_t' (aka 'unsigned long') → UBSan: store to misaligned address for type 'uintptr_t' (aka 'unsigned long')
I don't think of misaligned writes like this as "really" UB on x86, but if this UBSan flavor is at all useful, we should keep our codebase error-free...

We can use mozilla::LittleEndian::writeInt64() for this. Internally it's a memcpy for a fixed 8 bytes, which I believe is well-defined enough, and the compiler should find it easy to optimize.

nbp, would you be willing take a look?
Flags: needinfo?(nicolas.b.pierron)
Priority: -- → P3
(In reply to Jason Orendorff [:jorendorff] from comment #2)
> I don't think of misaligned writes like this as "really" UB on x86, but if
> this UBSan flavor is at all useful, we should keep our codebase error-free...

That is correct misaligned reads/writes are defined on x86 *but* not in C/C++. Doing so can limit the compilers ability to properly optimize or worse implement this operation correctly (in other words UB).
Blocks: 1284975
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Attachment #8955211 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8955211 [details] [diff] [review]
Part 2: Use writeUintptr to patch jitcode

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

Thanks.
Attachment #8955211 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8955210 [details] [diff] [review]
Part 1: Add methods to EndianUtils for pointer-sized integers

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

::: mfbt/EndianUtils.h
@@ +356,3 @@
>    }
>  
>    /** Read a uint64_t in ThisEndian endianness from |aPtr| and return it. */

Nothing mentions anywhere in this file that unaligned reads/writes are permitted by any of this interface.  Please modify this paragraph near the top of the file

"""
The classes LittleEndian and BigEndian expose static methods for reading and writing 16-, 32-, and 64-bit signed and unsigned integers in their respective endianness.  The naming scheme is:
"""

to

"""
The classes LittleEndian and BigEndian expose static methods for reading and writing 16-, 32-, and 64-bit signed and unsigned integers in their respective endianness.  The addresses read from or written to may be misaligned (although misaligned accesses may incur architecture-specific performance costs).  The naming scheme is:
"""

::: mfbt/tests/TestEndian.cpp
@@ +325,5 @@
>  main()
>  {
>    static const uint8_t unsigned_bytes[16] = {
> +    0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
> +    0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08

Could you split the 0-prefixing of things into a separate rev, please?  None of that has any effect on what this test does, so it's going to present pains to anyone doing archaeology on this file if left as part of this patch.  (Also made it slower to review all this.)

@@ +366,5 @@
> +    LittleEndian::readUint64(&unsigned_bytes[0]) == 0x0807060504030201ULL);
> +  MOZ_RELEASE_ASSERT(
> +    BigEndian::readUint64(&unsigned_bytes[0]) == 0x0102030405060708ULL);
> +
> +#ifdef HAVE_64BIT_BUILD

Could you instead have

  if (sizeof(uintptr_t) == 8) {
    ...
  } else {
    ...
  }

so we're not relying on a #define provided by the build system?  (Or at least I assume it would be provided by the build system.)  I often find it useful to test something in mfbt headers in minimal standalone code, and I can do that with "-I ..." tacked onto a minimal file's compilation command line.  Implicit dependencies like this tend to be silent footguns for that.

@@ +403,5 @@
> +  memcpy(&buffer[0], &signed_bytes[0], sizeof(buffer));
> +  LittleEndian::writeUintptr(&buffer[0], uintptr_t(0x0807060504030201ULL));
> +  MOZ_RELEASE_ASSERT(
> +    memcmp(&unsigned_bytes[0], &buffer[0], sizeof(uintptr_t)) == 0);
> +#ifndef HAVE_64BIT_BUILD

if (sizeof(uintptr_t) == 4)

@@ +408,5 @@
>    MOZ_RELEASE_ASSERT(
> +    memcmp(&signed_bytes[4], &buffer[4], 4) == 0);
> +#endif
> +
> +  memcpy(&buffer[0], &signed_bytes[0], sizeof(buffer));

Could you do a memset with 0xFF instead?  Then when you check in the 32-bit case that those trailing bytes weren't modified by the endian-write, it's obvious you're checking for that non-writing.

@@ +409,5 @@
> +    memcmp(&signed_bytes[4], &buffer[4], 4) == 0);
> +#endif
> +
> +  memcpy(&buffer[0], &signed_bytes[0], sizeof(buffer));
> +#ifdef HAVE_64BIT_BUILD

if (sizeof(uintptr_t) == 8)

@@ +410,5 @@
> +#endif
> +
> +  memcpy(&buffer[0], &signed_bytes[0], sizeof(buffer));
> +#ifdef HAVE_64BIT_BUILD
> +  BigEndian::writeUintptr(&buffer[0], 0x0102030405060708ULL);

Um.  So you memcpy into |buffer|, then you immediately overwrite it all?  I don't get how this makes any sense.  Should that memcpy be removed probably?

@@ +418,5 @@
> +  MOZ_RELEASE_ASSERT(
> +    memcmp(&unsigned_bytes[0], &buffer[0], sizeof(uintptr_t)) == 0);
> +#ifndef HAVE_64BIT_BUILD
> +  MOZ_RELEASE_ASSERT(
> +    memcmp(&signed_bytes[4], &buffer[4], 4) == 0);

I'd move this into the else I ask for above.

@@ +436,5 @@
>      LittleEndian::readInt64(&signed_bytes[0]) == int64_t(0xf8f7f6f5f4f3f2f1LL));
>    MOZ_RELEASE_ASSERT(
>      BigEndian::readInt64(&signed_bytes[0]) == int64_t(0xf1f2f3f4f5f6f7f8LL));
>  
> +#ifdef HAVE_64BIT_BUILD

if (siezof(intptr_t) == 8)

@@ +469,5 @@
>    BigEndian::writeInt64(&buffer[0], 0xf1f2f3f4f5f6f7f8LL);
>    MOZ_RELEASE_ASSERT(
>      memcmp(&signed_bytes[0], &buffer[0], sizeof(int64_t)) == 0);
>  
> +  memcpy(&buffer[0], &unsigned_bytes[0], sizeof(buffer));

memset of 0xFF, as mentioned above.

@@ +472,5 @@
>  
> +  memcpy(&buffer[0], &unsigned_bytes[0], sizeof(buffer));
> +  LittleEndian::writeIntptr(&buffer[0], intptr_t(0xf8f7f6f5f4f3f2f1LL));
> +  MOZ_RELEASE_ASSERT(
> +    memcmp(&signed_bytes[0], &buffer[0], sizeof(uintptr_t)) == 0);

sizeof(intptr_t) would be nicer

@@ +473,5 @@
> +  memcpy(&buffer[0], &unsigned_bytes[0], sizeof(buffer));
> +  LittleEndian::writeIntptr(&buffer[0], intptr_t(0xf8f7f6f5f4f3f2f1LL));
> +  MOZ_RELEASE_ASSERT(
> +    memcmp(&signed_bytes[0], &buffer[0], sizeof(uintptr_t)) == 0);
> +#ifndef HAVE_64BIT_BUILD

if (sizeof(intptr_t) == 4)

@@ +477,5 @@
> +#ifndef HAVE_64BIT_BUILD
> +  MOZ_RELEASE_ASSERT(
> +    memcmp(&unsigned_bytes[4], &buffer[4], 4) == 0);
> +#endif
> +  

trailing ws

@@ +478,5 @@
> +  MOZ_RELEASE_ASSERT(
> +    memcmp(&unsigned_bytes[4], &buffer[4], 4) == 0);
> +#endif
> +  
> +  memcpy(&buffer[0], &unsigned_bytes[0], sizeof(buffer));

memset 0xFF

@@ +479,5 @@
> +    memcmp(&unsigned_bytes[4], &buffer[4], 4) == 0);
> +#endif
> +  
> +  memcpy(&buffer[0], &unsigned_bytes[0], sizeof(buffer));
> +#ifdef HAVE_64BIT_BUILD

if (sizeof(intptr_t) == 8)

@@ +488,5 @@
> +  MOZ_RELEASE_ASSERT(
> +    memcmp(&signed_bytes[0], &buffer[0], sizeof(intptr_t)) == 0);
> +#ifndef HAVE_64BIT_BUILD
> +  MOZ_RELEASE_ASSERT(
> +    memcmp(&unsigned_bytes[4], &buffer[4], 4) == 0);

As before, put this in the else requested above.
Attachment #8955210 - Flags: review?(jwalden+bmo) → review+
> Um.  So you memcpy into |buffer|, then you immediately overwrite it all?  I don't get how this makes any sense.  Should that memcpy be removed probably?

The purpose of the memcpy is to make BigEndian::writeUintptr do some
observable work. Without it, the expected bytes are already sitting in the
buffer, even if writeUintptr does nothing, from the preceding code.

The existing tests are... perfunctory. My new tests are not much better,
admittedly.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6726bc85d05ef136bfcadfba96984ae5f5836fc8
Bug 1413049 - Part 1: Add methods to EndianUtils for pointer-sized integers. r=Waldo.

https://hg.mozilla.org/integration/mozilla-inbound/rev/960a3d703f4b30534edb77db380a45fd51de5088
Bug 1413049 - Part 1a: In a test, zero-pad hex integer constants to the width of their type. r=Waldo.

https://hg.mozilla.org/integration/mozilla-inbound/rev/71e1ae6a345ea3aec9693912acedc91785b15492
Bug 1413049 - Part 2: Use writeUintptr to patch jitcode. r=nbp.
Depends on: 1442967
Flags: needinfo?(nicolas.b.pierron)
You need to log in before you can comment on or make changes to this bug.