Open Bug 1772654 Opened 3 years ago Updated 3 years ago

dist/include/mozilla/gfx/Coord.h:144:41: runtime error: 2.14748e+09 is outside the range of representable values of type 'int'

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox103 --- affected

People

(Reporter: tsmith, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-undefined)

This was found by enabling the float-cast-overflow check in UBSan and running existing tests. This type of issue can create inconsistencies across platforms, architectures and optimization levels.

Found with m-c 20220603-68727ef04ccf

To enable this check add the following to your mozconfig:

ac_add_options --enable-undefined-sanitizer="float-cast-overflow"

This issue is found by the existing test: toolkit/components/windowcreator/test/test_window_open_position_constraint.html

/builds/worker/workspace/obj-build/dist/include/mozilla/gfx/Coord.h:144:41: runtime error: 2.14748e+09 is outside the range of representable values of type 'int'
    #0 0x7fa048f12799 in CalcSizeSpec(mozilla::dom::WindowFeatures const&, bool, mozilla::gfx::ScaleFactor<mozilla::CSSPixel, mozilla::DesktopPixel>) /builds/worker/checkouts/gecko/toolkit/components/windowwatcher/nsWindowWatcher.cpp
    #1 0x7fa048f0a7bb in nsWindowWatcher::OpenWindowInternal(mozIDOMWindowProxy*, nsTSubstring<char> const&, nsTSubstring<char> const&, nsTSubstring<char> const&, bool, bool, bool, nsIArray*, bool, bool, bool, nsPIWindowWatcher::PrintKind, nsDocShellLoadState*, mozilla::dom::BrowsingContext**) /builds/worker/checkouts/gecko/toolkit/components/windowwatcher/nsWindowWatcher.cpp:729:7
    #2 0x7fa048f10108 in nsWindowWatcher::OpenWindow2(mozIDOMWindowProxy*, nsTSubstring<char> const&, nsTSubstring<char> const&, nsTSubstring<char> const&, bool, bool, bool, nsISupports*, bool, bool, bool, nsPIWindowWatcher::PrintKind, nsDocShellLoadState*, mozilla::dom::BrowsingContext**) /builds/worker/checkouts/gecko/toolkit/components/windowwatcher/nsWindowWatcher.cpp:387:10
    #3 0x7fa03efc9414 in nsGlobalWindowOuter::OpenInternal(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, bool, bool, bool, bool, bool, nsIArray*, nsISupports*, nsDocShellLoadState*, bool, nsGlobalWindowOuter::PrintKind, mozilla::dom::BrowsingContext**) /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowOuter.cpp:7064:21
    #4 0x7fa03efce59f in nsGlobalWindowOuter::OpenJS(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::dom::BrowsingContext**) /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowOuter.cpp:5692:10
    #5 0x7fa03efce08e in nsGlobalWindowOuter::OpenOuter(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowOuter.cpp:5656:17
    #6 0x7fa03ef74743 in nsGlobalWindowInner::Open(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowInner.cpp:4098:3
    #7 0x7fa0407ab241 in mozilla::dom::Window_Binding::open(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/WindowBinding.cpp:2665:59
    #8 0x7fa041023e90 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::MaybeCrossOriginObjectThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3271:13
    #9 0x7fa04a9c6cd0 in CallJSNative /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:420:13
    #10 0x7fa04a9c6cd0 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:507:12
    #11 0x7fa04a9b53af in InternalCall /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:574:10
    #12 0x7fa04a9b53af in CallFromStack /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:578:10
    #13 0x7fa04a9b53af in Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3314:16
    #14 0x7fa04a99aad9 in js::RunScript(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:389:13
    #15 0x7fa04a9c6e0e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:539:13
    #16 0x7fa04a9c88ee in InternalCall /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:574:10
    #17 0x7fa04a9c88ee in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:605:8
    #18 0x7fa04967415f in Call /builds/worker/checkouts/gecko/js/src/vm/Interpreter.h:105:10
    #19 0x7fa04967415f in PromiseReactionJob(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/js/src/builtin/Promise.cpp:2242:10
    #20 0x7fa04a9c6cd0 in CallJSNative /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:420:13
    #21 0x7fa04a9c6cd0 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:507:12
    #22 0x7fa04a9c88ee in InternalCall /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:574:10
    #23 0x7fa04a9c88ee in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:605:8
    #24 0x7fa0493dbc05 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/CallAndConstruct.cpp:117:10
    #25 0x7fa03fde146c in mozilla::dom::PromiseJobCallback::Call(mozilla::dom::BindingCallContext&, JS::Handle<JS::Value>, mozilla::ErrorResult&) /builds/worker/workspace/obj-build/dom/bindings/PromiseBinding.cpp:35:8
    #26 0x7fa03c2443d7 in Call /builds/worker/workspace/obj-build/dist/include/mozilla/dom/PromiseBinding.h:88:12
    #27 0x7fa03c2443d7 in Call /builds/worker/workspace/obj-build/dist/include/mozilla/dom/PromiseBinding.h:101:12
    #28 0x7fa03c2443d7 in mozilla::PromiseJobRunnable::Run(mozilla::AutoSlowOperation&) /builds/worker/checkouts/gecko/xpcom/base/CycleCollectedJSContext.cpp:213:18
    #29 0x7fa03c224bc7 in mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint(bool) /builds/worker/checkouts/gecko/xpcom/base/CycleCollectedJSContext.cpp:674:17
    #30 0x7fa03c2258ff in mozilla::CycleCollectedJSContext::AfterProcessTask(unsigned int) /builds/worker/checkouts/gecko/xpcom/base/CycleCollectedJSContext.cpp:463:3
    #31 0x7fa03dcbd990 in XPCJSContext::AfterProcessTask(unsigned int) /builds/worker/checkouts/gecko/js/xpconnect/src/XPCJSContext.cpp:1481:28
    #32 0x7fa03c45bfe8 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1217:24
    #33 0x7fa03c465f44 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:465:10
    #34 0x7fa03da47b98 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:85:21
    #35 0x7fa03d8e57d1 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:380:10
    #36 0x7fa03d8e57d1 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:373:3
    #37 0x7fa03d8e57d1 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:355:3
    #38 0x7fa044248d57 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:137:27
    #39 0x7fa048fc46c7 in XRE_RunAppShell() /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:875:20
    #40 0x7fa03d8e57d1 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:380:10
    #41 0x7fa03d8e57d1 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:373:3
    #42 0x7fa03d8e57d1 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:355:3
    #43 0x7fa048fc386b in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:734:34
    #44 0x558f625ce825 in content_process_main(mozilla::Bootstrap*, int, char**) /builds/worker/checkouts/gecko/browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
    #45 0x558f625cebd6 in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:338:18
    #46 0x7fa062e73b96 in __libc_start_main /tmp/glibc/csu/../csu/libc-start.c:310
    #47 0x558f6250ec80 in _start (/builds/worker/workspace/build/application/firefox/firefox+0x72c80) (BuildId: 42cda71a61972be2e9518daa796551945b598f1e)
Severity: -- → S3

This issue is currently triggered while running existing tests with the 'float-cast-overflow' UBSan check enabled. This issue will need to be addressed before the check can be enabled by default.

If it requires too much effort to fix immediately please ni? me and let me know. If necessary it will be added to a suppression list. Thank you :)

Flags: needinfo?(enndeakin)

Redirect to Emilio who seems to have added the Rounded() calls here.

Flags: needinfo?(enndeakin) → needinfo?(emilio)

That code is fairly generic so basically any code calling Rounded() with an out-on-range float:

Layout has similar callers:

We need a real plan to deal with this, I could fix the specific caller but it'd be a whack-a-mole. We have a lot of places where we assume that float to integer casts truncate. We could clamp the out-of-range floating point values, but that seems to generate worse code:

Tyson, do you know how are we dealing with similar issues? I think we should probably fix the couple callers linked above and check performance.

Component: Window Management → Graphics
Flags: needinfo?(emilio) → needinfo?(twsmith)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Tyson, do you know how are we dealing with similar issues? I think we should probably fix the couple callers linked above and check performance.

I'm not sure how bad the "whack-a-mole" approach will be, you likely have a better sense of that than I do (I'm always happy to keep point out moles :) ). Since this is a test and not a fuzzer found bug maybe it won't be too bad? I don't think the fuzzers have reported anything similar, but they might not have coverage. I'm not aware of any severe cases of this so far.

But perhaps it would be better to add a suppression in this case? What is the worst outcome of a float-cast-overflow in this case?

Flags: needinfo?(twsmith) → needinfo?(emilio)

(In reply to Tyson Smith [:tsmith] from comment #4)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Tyson, do you know how are we dealing with similar issues? I think we should probably fix the couple callers linked above and check performance.

I'm not sure how bad the "whack-a-mole" approach will be, you likely have a better sense of that than I do (I'm always happy to keep point out moles :) ). Since this is a test and not a fuzzer found bug maybe it won't be too bad? I don't think the fuzzers have reported anything similar, but they might not have coverage. I'm not aware of any severe cases of this so far.

So... I think this is likely to get hit in a bunch of places in our rendering pipeline (basically anywhere we end up dividing by a very small float or something).

But perhaps it would be better to add a suppression in this case? What is the worst outcome of a float-cast-overflow in this case?

So if I understand correctly, float cast overflow behavior depends how the compiler implements float-to-int casts.

On all the architectures we care about, it seems to be implemented as saturation (and we rely on that).

It seems that compilers are fairly consistent at how they evaluate these, though in theory we could end up with wrong integers (but in any case I don't think it should cause any badness other than "weird behavior in edge cases")...

Flags: needinfo?(emilio)
You need to log in before you can comment on or make changes to this bug.