1431874 - UBSan: addition of unsigned offset to pointer overflowed in js/src/ctypes/CTypes.cpp:3211

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tyson Smith [:tsmith]]

This happens on launch when built with -fsanitize=pointer-overflow

changeset: 399986:6bb6f3b25f9f

Marking as s-s to be safe.

/mozilla-central/js/src/ctypes/CTypes.cpp:3211:46: runtime error: addition of unsigned offset to 0x7f0d57f6a178 overflowed to 0x7f0d57f6a177
    #0 0x7f0d2da48b2f in unsigned long js::ctypes::strnlen<char>(char const*, unsigned long) /mozilla-central/js/src/ctypes/CTypes.cpp:3211:46
    #1 0x7f0d2da48411 in js::ctypes::ReadStringCommon(JSContext*, JS::TwoByteCharsZ (*)(JSContext*, JS::UTF8Chars, unsigned long*), unsigned int, JS::Value*, char const*) /mozilla-central/js/src/ctypes/CTypes.cpp:7987:21
    #2 0x7f0d2db82079 in CallJSNative /mozilla-central/js/src/jscntxtinlines.h:291:15
    #3 0x7f0d2db82079 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /mozilla-central/js/src/vm/Interpreter.cpp:473
    #4 0x7f0d2db82f7f in InternalCall(JSContext*, js::AnyInvokeArgs const&) /mozilla-central/js/src/vm/Interpreter.cpp:522:12
    #5 0x7f0d2db68586 in Interpret(JSContext*, js::RunState&) /mozilla-central/js/src/vm/Interpreter.cpp:3096:18
    #6 0x7f0d2db4b0d8 in js::RunScript(JSContext*, js::RunState&) /mozilla-central/js/src/vm/Interpreter.cpp:423:12
    #7 0x7f0d2db82189 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /mozilla-central/js/src/vm/Interpreter.cpp:495:15
    #8 0x7f0d2db82f7f in InternalCall(JSContext*, js::AnyInvokeArgs const&) /mozilla-central/js/src/vm/Interpreter.cpp:522:12
    #9 0x7f0d2db8315a in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /mozilla-central/js/src/vm/Interpreter.cpp:541:10
    #10 0x7f0d2dc7b09d in js::Call(JSContext*, JS::Handle<JS::Value>, JSObject*, JS::MutableHandle<JS::Value>) /mozilla-central/js/src/vm/Interpreter.h:94:12
    #11 0x7f0d2e772ebc in MaybeCallMethod(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /mozilla-central/js/src/jsobj.cpp:3051:12
    #12 0x7f0d2e772a40 in JS::OrdinaryToPrimitive(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>) /mozilla-central/js/src/jsobj.cpp
    #13 0x7f0d2e773e35 in js::ToPrimitiveSlow(JSContext*, JSType, JS::MutableHandle<JS::Value>) /mozilla-central/js/src/jsobj.cpp:3181:12
    #14 0x7f0d2dba050d in js::ToPrimitive(JSContext*, JS::MutableHandle<JS::Value>) /mozilla-central/js/src/jsobj.h:996:12
    #15 0x7f0d2db6f117 in AddOperation /mozilla-central/js/src/vm/Interpreter.cpp:1489:10
    #16 0x7f0d2db6f117 in Interpret(JSContext*, js::RunState&) /mozilla-central/js/src/vm/Interpreter.cpp:2570
    #17 0x7f0d2db4b0d8 in js::RunScript(JSContext*, js::RunState&) /mozilla-central/js/src/vm/Interpreter.cpp:423:12
    #18 0x7f0d2db82189 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /mozilla-central/js/src/vm/Interpreter.cpp:495:15
    #19 0x7f0d2db82f7f in InternalCall(JSContext*, js::AnyInvokeArgs const&) /mozilla-central/js/src/vm/Interpreter.cpp:522:12
    #20 0x7f0d2db8315a in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /mozilla-central/js/src/vm/Interpreter.cpp:541:10
    #21 0x7f0d2ebcff32 in js::CallSelfHostedFunction(JSContext*, JS::Handle<js::PropertyName*>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /mozilla-central/js/src/vm/SelfHosting.cpp:1728:12
    #22 0x7f0d2e8b4691 in AsyncFunctionResume(JSContext*, JS::Handle<js::PromiseObject*>, JS::Handle<JS::Value>, ResumeKind, JS::Handle<JS::Value>) /mozilla-central/js/src/vm/AsyncFunction.cpp:191:10
    #23 0x7f0d2dd39449 in AsyncFunctionPromiseReactionJob(JSContext*, JS::Handle<PromiseReactionRecord*>, JS::MutableHandle<JS::Value>) /mozilla-central/js/src/builtin/Promise.cpp:1098:14
    #24 0x7f0d2dd382e8 in PromiseReactionJob(JSContext*, unsigned int, JS::Value*) /mozilla-central/js/src/builtin/Promise.cpp:1199:16
    #25 0x7f0d2db82079 in CallJSNative /mozilla-central/js/src/jscntxtinlines.h:291:15
    #26 0x7f0d2db82079 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /mozilla-central/js/src/vm/Interpreter.cpp:473
    #27 0x7f0d2db82f7f in InternalCall(JSContext*, js::AnyInvokeArgs const&) /mozilla-central/js/src/vm/Interpreter.cpp:522:12
    #28 0x7f0d2db8315a in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /mozilla-central/js/src/vm/Interpreter.cpp:541:10
    #29 0x7f0d2e5b6917 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /mozilla-central/js/src/jsapi.cpp:3029:12
    #30 0x7f0d24a17df6 in mozilla::dom::PromiseJobCallback::Call(JSContext*, JS::Handle<JS::Value>, mozilla::ErrorResult&) /mozilla-central/objdir-ff-ubsan/dom/bindings/PromiseBinding.cpp:21:8
    #31 0x7f0d20546895 in mozilla::dom::PromiseJobCallback::Call(mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) /mozilla-central/objdir-ff-ubsan/dist/include/mozilla/dom/PromiseBinding.h:89:12
    #32 0x7f0d2054647c in mozilla::dom::PromiseJobCallback::Call(char const*) /mozilla-central/objdir-ff-ubsan/dist/include/mozilla/dom/PromiseBinding.h:104:12
    #33 0x7f0d205451c9 in mozilla::PromiseJobRunnable::Run() /mozilla-central/xpcom/base/CycleCollectedJSContext.cpp:208:18
    #34 0x7f0d27758870 in mozilla::dom::Promise::PerformMicroTaskCheckpoint() /mozilla-central/dom/promise/Promise.cpp:533:29
    #35 0x7f0d2052546b in mozilla::CycleCollectedJSContext::AfterProcessTask(unsigned int) /mozilla-central/xpcom/base/CycleCollectedJSContext.cpp:361:7
    #36 0x7f0d227df59f in XPCJSContext::AfterProcessTask(unsigned int) /mozilla-central/js/xpconnect/src/XPCJSContext.cpp:1249:30
    #37 0x7f0d2071416a in nsThread::ProcessNextEvent(bool, bool*) /mozilla-central/xpcom/threads/nsThread.cpp:1056:24
    #38 0x7f0d2074ec0a in NS_ProcessNextEvent(nsIThread*, bool) /mozilla-central/xpcom/threads/nsThreadUtils.cpp:517:10
    #39 0x7f0d219d3f31 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /mozilla-central/ipc/glue/MessagePump.cpp:97:21
    #40 0x7f0d2183e910 in MessageLoop::Run() /mozilla-central/ipc/chromium/src/base/message_loop.cc:299:3
    #41 0x7f0d27c033f5 in nsBaseAppShell::Run() /mozilla-central/widget/nsBaseAppShell.cpp:157:27
    #42 0x7f0d2d5c2047 in nsAppStartup::Run() /mozilla-central/toolkit/components/startup/nsAppStartup.cpp:288:30
    #43 0x7f0d2d795f88 in XREMain::XRE_mainRun() /mozilla-central/toolkit/xre/nsAppRunner.cpp:4702:22
    #44 0x7f0d2d797e3f in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /mozilla-central/toolkit/xre/nsAppRunner.cpp:4841:8
    #45 0x7f0d2d798cb1 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /mozilla-central/toolkit/xre/nsAppRunner.cpp:4933:21
    #46 0x51851e in do_main(int, char**, char**) /mozilla-central/browser/app/nsBrowserApp.cpp:231:22
    #47 0x517d24 in main /mozilla-central/browser/app/nsBrowserApp.cpp:304:16
    #48 0x7f0d57de71c0 in __libc_start_main /build/glibc-itYbWN/glibc-2.26/csu/../csu/libc-start.c:308
    #49 0x4207a9 in _start (/mozilla-central/objdir-ff-ubsan/dist/bin/firefox+0x4207a9)

Comment 1

[Christian Holler (:decoder)]

This looks intended:

7959  size_t maxLength = -1;
...
7987 size_t length = strnlen(bytes, maxLength);


which overflows `begin + max` here:

3211  for (const CharType* s = begin; s != begin + max; ++s)


What this simply causes is the strnlen to behave like strlen (lifting the limitation given by n). As it is well-defined (I think?), it's going to be hard to argue about changing this. Maybe we should make a blacklist for pointer overflows, it shouldn't be too common.

Comment 2

[Jeff Walden [:Waldo]]

Addition to a pointer to form a pointer that isn't within the range of the underlying array (or one past it) is undefined behavior, C++14 [expr.add]p5: "If both the pointer operand and the result point to elements of the same array object, or one past the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined."

This looks pretty easily fixable by looping using an index instead of a pointer.  Compiler should be able to optimize it to whatever's actually best at the machine level (that, and/or worrying about ctypes code perf micro-issues is premature).  I'll get a patch.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch

Comment 4

[Christian Holler (:decoder)]

(In reply to Jeff Walden [:Waldo] (I'm baaaaaaack...) from comment #2)
> Addition to a pointer to form a pointer that isn't within the range of the
> underlying array (or one past it) is undefined behavior, C++14 [expr.add]p5:
> "If both the pointer operand and the result point to elements of the same
> array object, or one past the last element of the array object, the
> evaluation shall not produce an overflow; otherwise, the behavior is
> undefined."

Does that first sentence mean that if the overflowed value points to a different array, it is defined behavior again? I also don't get the last sentence somehow :D I wonder why people wrote the code like this in the first place, since the solution with the index that you implemented is much easier as well.

Comment 5

[Jeff Walden [:Waldo]]

(In reply to Christian Holler (:decoder) from comment #4)
> Does that first sentence mean that if the overflowed value points to a
> different array, it is defined behavior again?

No.  Only if it's part of literally the same array is it defined.

However -- if you had

  struct S
  {
    int arr1[2];
    bool splitter; // so adding offsetof(...) doesn't point one past |arr1|
    int arr2[2];
  };
  S instance;
  int* ptr = instance.arr1;

and then did this:

  uintptr_t arr1Ptr = reinterpret_cast<uintptr_t>(ptr);
  uintptr_t arr2Ptr = arr1Ptr + offsetof(S, arr2); // address of instance.arr2
  int* ptr2 = reinterpret_cast<int*>(arr2Ptr);

then you *could* safely use |*ptr2| and |ptr2[0]| and |ptr2[1]|.  Pointer arithmetic that *doesn't* use actual pointers, produces address values on which well-defined math can be performed.  And if you convert the resulting numeric address back to pointer, that pointer *can* safely be dereferenced so long as the underlying memory does contain the pointee type.  It's only using pointers directly, that has you go wrong.

Incidentally, this also means that all those structs we have like

  struct S
  {
    ...
    T trailing[1];
  };
  void* mem = malloc(sizeof(S) + (trailingCount - 1) * sizeof(T));
  S* s = reinterpret_cast<S*>(mem);

with trailing elements, where people then do things like |s->trailing[index]| to use that faked-up array, are actually undefined behavior.  It is an uphill battle to convince people that playing with fire like this is wrong.

> I wonder why people wrote the code like this in the
> first place, since the solution with the index that you implemented is much
> easier as well.

My guess is it's an arguable premature optimization.

There are two ways to implement this sort of loop in assembly.  You can have a loop that increments a pointer by sizeof(T).  You can also have a loop that uses a constant base pointer and then uses x86-style addressing to combine that base pointer in one register, an increment-by-1 index in another, and a power-of-two constant multiple encoded in the instruction -- |mov edx, [esi+4*ebx]| in Intel syntax, say.

Last I heard, the latter was usually faster on modern architectures.  (You probably should not entirely trust that claim.)  So possibly in an effort to "help out" the compiler, people wrote this as working with pointers.  But the reality is optimizing compilers now do loop induction stuffs so that the two styles of loop are interpreted exactly identically, and then the compiler lowers it to the assembly that works best for the target architecture.

(Semi-relatedly, there's also a little problem that idiomatic C++ really wants you to use iterators for everything.  You *can* implement iterators as base-pointer plus index, but it's probably the less common way it's done.  So people might tend to work with pointer-ranges more in looping, but if you can't construct the faulty endpoint, you're gonna inadvertently do UB.)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/352d2ed92fef
Loop using a size_t index, not a pointer, to avoid undefined behavior computing an end pointer possibly outside the range of the underlying data.  r=jonco

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/352d2ed92fef