1588938 - undefined shift in intl/icu/source/common/ubidiln.cpp:666

Status: NEW

(Core :: JavaScript: Internationalization API, defect, P3)

Description

[Tyson Smith [:tsmith]]

This is triggered when running gtests with an UBSan build.

To enable this check add the following to your mozconfig:

ac_add_options --enable-address-sanitizer
ac_add_options --enable-undefined-sanitizer="shift"
ac_add_options --disable-jemalloc

REFTEST TEST-START | file://tests/reftest/tests/dom/base/crashtests/416734-1.html
REFTEST TEST-LOAD | file://tests/reftest/tests/dom/base/crashtests/416734-1.html | 143 / 3837 (3%)
src/intl/icu/source/common/ubidiln.cpp:666:17: runtime error: left shift of 2 by 31 places cannot be represented in type 'int32_t' (aka 'int')
    #0 0x7f9e1c264e4b in ubidi_getRuns_64 src/intl/icu/source/common/ubidiln.cpp:666:17
    #1 0x7f9e1c263afa in ubidi_countRuns_64 src/intl/icu/source/common/ubidiln.cpp:355:5
    #2 0x7f9e22f87741 in nsBidi::CountRuns(int*) src/layout/base/nsBidi.cpp:11:16
    #3 0x7f9e22f8b1f5 in nsBidiPresUtils::ResolveParagraph(BidiParagraphData*) src/layout/base/nsBidiPresUtils.cpp:881:14
    #4 0x7f9e22f8a37c in ResolveParagraphWithinBlock src/layout/base/nsBidiPresUtils.cpp:1473:3
    #5 0x7f9e22f8a37c in nsBidiPresUtils::TraverseFrames(nsIFrame*, BidiParagraphData*) src/layout/base/nsBidiPresUtils.cpp:1346:15
    #6 0x7f9e22f89a64 in nsBidiPresUtils::TraverseFrames(nsIFrame*, BidiParagraphData*) src/layout/base/nsBidiPresUtils.cpp:1392:9
    #7 0x7f9e22f89a64 in nsBidiPresUtils::TraverseFrames(nsIFrame*, BidiParagraphData*) src/layout/base/nsBidiPresUtils.cpp:1392:9
    #8 0x7f9e22f89a64 in nsBidiPresUtils::TraverseFrames(nsIFrame*, BidiParagraphData*) src/layout/base/nsBidiPresUtils.cpp:1392:9
    #9 0x7f9e22f87edd in nsBidiPresUtils::Resolve(nsBlockFrame*) src/layout/base/nsBidiPresUtils.cpp:851:5
    #10 0x7f9e2310ab21 in ResolveBidi src/layout/generic/nsBlockFrame.cpp:7559:10
    #11 0x7f9e2310ab21 in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) src/layout/generic/nsBlockFrame.cpp:1241:54
    #12 0x7f9e2312889b in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, mozilla::ReflowInput&, nsReflowStatus&, mozilla::BlockReflowInput&) src/layout/generic/nsBlockReflowContext.cpp:291:11
    #13 0x7f9e2311ec54 in nsBlockFrame::ReflowBlockFrame(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:3643:11
    #14 0x7f9e2311b427 in nsBlockFrame::ReflowLine(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:2994:5
    #15 0x7f9e23112849 in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) src/layout/generic/nsBlockFrame.cpp:2537:7
    #16 0x7f9e2310af2b in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) src/layout/generic/nsBlockFrame.cpp:1280:3
    #17 0x7f9e23158de4 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, nsIFrame::ReflowChildFlags, nsReflowStatus&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:910:14
    #18 0x7f9e23157e5b in nsCanvasFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) src/layout/generic/nsCanvasFrame.cpp:726:5
    #19 0x7f9e23158de4 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, nsIFrame::ReflowChildFlags, nsReflowStatus&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:910:14
    #20 0x7f9e2323966e in nsHTMLScrollFrame::ReflowScrolledFrame(mozilla::ScrollReflowInput*, bool, bool, mozilla::ReflowOutput*) src/layout/generic/nsGfxScrollFrame.cpp:644:3
    #21 0x7f9e2323aeb1 in nsHTMLScrollFrame::ReflowContents(mozilla::ScrollReflowInput*, mozilla::ReflowOutput const&) src/layout/generic/nsGfxScrollFrame.cpp:758:3
    #22 0x7f9e2323ee7b in nsHTMLScrollFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) src/layout/generic/nsGfxScrollFrame.cpp:1160:3
    #23 0x7f9e230fb201 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, nsIFrame::ReflowChildFlags, nsReflowStatus&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:950:14
    #24 0x7f9e230fa7a0 in mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) src/layout/generic/ViewportFrame.cpp:299:7
    #25 0x7f9e22f1879e in mozilla::PresShell::DoReflow(nsIFrame*, bool, mozilla::OverflowChangedTracker*) src/layout/base/PresShell.cpp:9236:11
    #26 0x7f9e22f2bbf7 in mozilla::PresShell::ProcessReflowCommands(bool) src/layout/base/PresShell.cpp:9406:24
    #27 0x7f9e22f2a313 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4172:11
    #28 0x7f9e22fe8602 in FlushPendingNotifications src/obj-firefox/dist/include/mozilla/PresShell.h:1443:5
    #29 0x7f9e22fe8602 in nsDocumentViewer::LoadComplete(nsresult) src/layout/base/nsDocumentViewer.cpp:1032:16
    #30 0x7f9e2566701c in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) src/docshell/base/nsDocShell.cpp:6478:20
    #31 0x7f9e2566624d in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp:6256:7
    #32 0x7f9e2566ac8f in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp
    #33 0x7f9e1db43850 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) src/uriloader/base/nsDocLoader.cpp:1352:3
    #34 0x7f9e1db427dc in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:911:14
    #35 0x7f9e1db3eadf in nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:731:9
    #36 0x7f9e1db412e3 in nsDocLoader::OnStopRequest(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:619:5
    #37 0x7f9e1db4236c in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp
    #38 0x7f9e1c038e37 in mozilla::net::nsLoadGroup::NotifyRemovalObservers(nsIRequest*, nsresult) src/netwerk/base/nsLoadGroup.cpp:595:22
    #39 0x7f9e1c03bc77 in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) src/netwerk/base/nsLoadGroup.cpp:502:10
    #40 0x7f9e1ec8364f in mozilla::dom::Document::DoUnblockOnload() src/dom/base/Document.cpp:10748:18
    #41 0x7f9e1ec353ec in mozilla::dom::Document::UnblockOnload(bool) src/dom/base/Document.cpp:10680:9
    #42 0x7f9e1ec5c0bc in mozilla::dom::Document::DispatchContentLoadedEvents() src/dom/base/Document.cpp:7233:3
    #43 0x7f9e1ed25a64 in applyImpl<mozilla::dom::Document, void (mozilla::dom::Document::*)()> src/obj-firefox/dist/include/nsThreadUtils.h:1124:12
    #44 0x7f9e1ed25a64 in apply<mozilla::dom::Document, void (mozilla::dom::Document::*)()> src/obj-firefox/dist/include/nsThreadUtils.h:1130:12
    #45 0x7f9e1ed25a64 in mozilla::detail::RunnableMethodImpl<mozilla::dom::Document*, void (mozilla::dom::Document::*)(), true, (mozilla::RunnableKind)0>::Run() src/obj-firefox/dist/include/nsThreadUtils.h:1176:13
    #46 0x7f9e1bdbbbc1 in mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:295:32
    #47 0x7f9e1bdf0dd7 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1225:14
    #48 0x7f9e1bdf8d7c in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:486:10
    #49 0x7f9e1cd91f3a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:88:21
    #50 0x7f9e1ccc9a47 in RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
    #51 0x7f9e1ccc9a47 in RunHandler src/ipc/chromium/src/base/message_loop.cc:308:3
    #52 0x7f9e1ccc9a47 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290:3
    #53 0x7f9e22a547d8 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:137:27
    #54 0x7f9e2604e716 in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:934:20
    #55 0x7f9e1ccc9a47 in RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
    #56 0x7f9e1ccc9a47 in RunHandler src/ipc/chromium/src/base/message_loop.cc:308:3
    #57 0x7f9e1ccc9a47 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290:3
    #58 0x7f9e2604dd71 in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:769:34
    #59 0x564911beb338 in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
    #60 0x564911beb338 in main src/browser/app/nsBrowserApp.cpp:272:18

Comment 1

[Jason Orendorff [:jorendorff]]

The line in question reads:

                ADD_ODD_BIT_FROM_LEVEL(runs[i].logicalStart, levels[runs[i].logicalStart]);

The macro definition:

#define ADD_ODD_BIT_FROM_LEVEL(x, level)  ((x)|=((int32_t)((level)&1)<<31))

I don't think that is UB. The value being left-shifted must be 1 or 0. Tyson, could this be a false positive?

Comment 3

[Tyson Smith [:tsmith]]

(In reply to Jason Orendorff [:jorendorff] from comment #1)

Tyson, could this be a false positive?

I doubt it is a false positive[1].

Shifting a signed int with the value of 1 left by 31 is undefined. So I'm guessing the &1 is being optimized and since we are dealing with UB we are seeing oddness.

[1] https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#issue-suppression

Comment 4

[Jason Orendorff [:jorendorff]]

Oh, I see! That makes sense.

Or it seemed to. Then I looked closer, and now I'm very confused.

The bitmask was added in this bug. The problem being solved was about the value 2 being left-shifted by 31.

At the time, a contributor questioned whether the signed left shift was well-defined, and seemed to conclude that it was, i.e. int32_t(1)<<31 is well-defined in C++.

Jeff, is that correct?

Comment 5

[Jason Orendorff [:jorendorff]]

Jeff pointed out on IRC that this behavior is well-defined since C++14. The page even mentions the particular case of 1<<31:

For signed and non-negative a, if a * 2^b is representable in the unsigned version of the return type, then that value, converted to signed, is the value of a << b (this makes it legal to create INT_MIN as 1<<31); otherwise the behavior is undefined. (since C++14) (until C++20)

(In C++20, left shift is always well-defined. There was no change to 1<<31 in C++20.)

Tyson, given that we require C++14, should we worry about this? What are our goals with UBSan?

Comment 6

[Jeff Walden [:Waldo]]

Arguably, UBSan should generate different instrumentation for operations where UB has changed in different C++ editions over time. Given ubsan is not supposed to be about the specific compiler in use, but is meant to detect issues that could arise under the hypothetical fully-spec-compliant compiler, it probably should be exactly as pedantic as whichever of C++11, C++14, C++17, or C++2a is in force.

In practice, ICU itself I don't believe has specific C++ edition requirements beyond possibly C++11. And because they view building the code as something that embedders will frequently customize for their own build systems -- we do this in config/external/icu/ ourselves -- unless there's something like

#if __cplusplus < 20140101LL
#  error "You must compile ICU as at least C++14"
#endif

that's part of every C++ translation unit in ICU, it is somewhat risky to rely on our happening to use new enough C++.

This is the sort of thing that should be pretty easy to fix upstream to work even under C++11, IMO, and it wouldn't be difficult to get them to land such patch, then carry that patch on our side til the next update incorporates it.

Comment 7

[Tyson Smith [:tsmith]]

(In reply to Jason Orendorff [:jorendorff] from comment #5)

Tyson, given that we require C++14, should we worry about this? What are our goals with UBSan?

The goal is to enable UBSan checks while using few or no suppression lists. This does mean issues like this fall in to the "no big deal" category and it is tempting to just add it to the suppression list and move on. If this is easily fixable I think it should be fixed in-tree and upstream. This will make the issue disappear for good (hopefully) unlike the suppression list which can become an unmaintained dumping ground and will not provide the benefit for other projects.

Comment 8

[Jason Orendorff [:jorendorff]]

OK. Setting to P3 as this is definitely not an actual UB bug (for us). Feel free to suppress the issue or fix in-tree and upstream; or attempt to convince Jeff to do so.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.