1614512 - load of value 4, which is not a valid value for type 'mozilla::Side' in objdir-ff-ubsan/dist/include/mozilla/EnumeratedRange.h

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Tyson Smith [:tsmith]]

This is triggered on launch 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="enum"
ac_add_options --disable-jemalloc

Found with m-c 20200209-be2a7d1a4d0d

objdir-ff-ubsan/dist/include/mozilla/EnumeratedRange.h:170:52: runtime error: load of value 4, which is not a valid value for type 'mozilla::Side'
    #0 0x7f2f26a307fc in mozilla::detail::EnumeratedRange<mozilla::Side> mozilla::MakeEnumeratedRange<mozilla::Side>(mozilla::Side, mozilla::Side) objdir-ff-ubsan/dist/include/mozilla/EnumeratedRange.h:170:52
    #1 0x7f2f2b43ea2b in nsStyleBorder::nsStyleBorder(mozilla::dom::Document const&) layout/style/nsStyleStruct.cpp:431:26
    #2 0x7f2f32db5937 in style::gecko_properties::_$LT$impl$u20$style..gecko_bindings..structs..root..mozilla..GeckoBorder$GT$::default::h37b1b7d67c6fe563 objdir-ff-ubsan/x86_64-unknown-linux-gnu/release/build/style-212926e50dbabe86/out/gecko_properties.rs:17764:12
    #3 0x7f2f332b7a93 in style::gecko_properties::ComputedValues::default_values::he8506dd639d503d6 objdir-ff-ubsan/x86_64-unknown-linux-gnu/release/build/style-212926e50dbabe86/out/gecko_properties.rs:305:12
    #4 0x7f2f33117312 in style::gecko::media_queries::Device::new::h537a06e83c7e2dee servo/components/style/gecko/media_queries.rs:87:28
    #5 0x7f2f33520e94 in style::gecko::data::PerDocumentStyleData::new::haf581417bda4c7e6 servo/components/style/gecko/data.rs:147:21
    #6 0x7f2f329d2187 in Servo_StyleSet_Init servo/ports/geckolib/glue.rs:3995:24
    #7 0x7f2f2b3e3a02 in mozilla::ServoStyleSet::ServoStyleSet(mozilla::dom::Document&) layout/style/ServoStyleSet.cpp:96:17
    #8 0x7f2f26d40c14 in mozilla::detail::UniqueSelector<mozilla::ServoStyleSet>::SingleObject mozilla::MakeUnique<mozilla::ServoStyleSet, mozilla::dom::Document&>(mozilla::dom::Document&) objdir-ff-ubsan/dist/include/mozilla/UniquePtr.h:633:27
    #9 0x7f2f26c9cf1f in mozilla::dom::Document::Init() dom/base/Document.cpp:2396:15
    #10 0x7f2f292c4833 in nsHTMLDocument::Init() dom/html/nsHTMLDocument.cpp:144:27
    #11 0x7f2f292c441f in NS_NewHTMLDocument(mozilla::dom::Document**, bool) dom/html/nsHTMLDocument.cpp:111:22
    #12 0x7f2f2bd6d8b5 in nsContentDLF::CreateBlankDocument(nsILoadGroup*, nsIPrincipal*, nsIPrincipal*, nsDocShell*) layout/build/nsContentDLF.cpp:232:22
    #13 0x7f2f2e6d2937 in nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal*, nsIPrincipal*, nsIContentSecurityPolicy*, nsIURI*, bool, bool, mozilla::dom::WindowGlobalChild*) docshell/base/nsDocShell.cpp:6588:16
    #14 0x7f2f2e6d34aa in nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal*, nsIPrincipal*, nsIContentSecurityPolicy*) docshell/base/nsDocShell.cpp:6643:10
    #15 0x7f2f2e7a0b89 in mozilla::AppWindow::Initialize(nsIAppWindow*, nsIAppWindow*, nsIURI*, int, int, bool, nsIRemoteTab*, mozIDOMWindowProxy*, nsWidgetInitData&) xpfe/appshell/AppWindow.cpp:297:21
    #16 0x7f2f2e7bab2d in nsAppShellService::JustCreateTopWindow(nsIAppWindow*, nsIURI*, unsigned int, int, int, bool, nsIRemoteTab*, mozIDOMWindowProxy*, mozilla::AppWindow**) xpfe/appshell/nsAppShellService.cpp:679:25
    #17 0x7f2f2e7bb732 in nsAppShellService::CreateTopLevelWindow(nsIAppWindow*, nsIURI*, unsigned int, int, int, nsIRemoteTab*, mozIDOMWindowProxy*, nsIAppWindow**) xpfe/appshell/nsAppShellService.cpp:172:8
    #18 0x7f2f2ef14342 in nsAppStartup::CreateChromeWindow(nsIWebBrowserChrome*, unsigned int, nsIRemoteTab*, mozIDOMWindowProxy*, unsigned long, bool*, nsIWebBrowserChrome**) toolkit/components/startup/nsAppStartup.cpp:635:15
    #19 0x7f2f2f084359 in nsWindowWatcher::CreateChromeWindow(nsTSubstring<char> const&, nsIWebBrowserChrome*, unsigned int, nsIRemoteTab*, mozIDOMWindowProxy*, unsigned long, nsIWebBrowserChrome**) toolkit/components/windowwatcher/nsWindowWatcher.cpp:419:33
    #20 0x7f2f2f0808fe in nsWindowWatcher::OpenWindowInternal(mozIDOMWindowProxy*, char const*, char const*, char const*, bool, bool, bool, nsIArray*, bool, bool, bool, nsDocShellLoadState*, mozilla::dom::BrowsingContext**) toolkit/components/windowwatcher/nsWindowWatcher.cpp:904:12
    #21 0x7f2f2f07f11f in nsWindowWatcher::OpenWindow(mozIDOMWindowProxy*, char const*, char const*, char const*, nsISupports*, mozIDOMWindowProxy**) toolkit/components/windowwatcher/nsWindowWatcher.cpp:292:3
    #22 0x7f2f22834249 in NS_InvokeByIndex xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:106
    #23 0x7f2f24fa0f09 in CallMethodHelper::Invoke() js/xpconnect/src/XPCWrappedNative.cpp:1643:10
    #24 0x7f2f24fa0f09 in CallMethodHelper::Call() js/xpconnect/src/XPCWrappedNative.cpp:1184:19
    #25 0x7f2f24fa0f09 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) js/xpconnect/src/XPCWrappedNative.cpp:1150:23
    #26 0x7f2f24fa4e5c in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) js/xpconnect/src/XPCWrappedNativeJSOps.cpp:947:10
    #27 0x7f2f2f403ff2 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) js/src/vm/Interpreter.cpp:470:13
    #28 0x7f2f2f403ff2 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) js/src/vm/Interpreter.cpp:562:12
    #29 0x7f2f2f404f32 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) js/src/vm/Interpreter.cpp:625:10
    #30 0x7f2f2f3eef62 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3042:16
    #31 0x7f2f2f3d1e2d in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:442:10
    #32 0x7f2f2f403e60 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) js/src/vm/Interpreter.cpp:597:13
    #33 0x7f2f2f404f32 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) js/src/vm/Interpreter.cpp:625:10
    #34 0x7f2f2f405115 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) js/src/vm/Interpreter.cpp:642:8
    #35 0x7f2f2f63dd56 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:2734:10
    #36 0x7f2f24f96a98 in nsXPCWrappedJS::CallMethod(unsigned short, nsXPTMethodInfo const*, nsXPTCMiniVariant*) js/xpconnect/src/XPCWrappedJSClass.cpp:959:17
    #37 0x7f2f22835bf0 in PrepareAndDispatch xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp:125:37
    #38 0x7f2f228349da in SharedStub (objdir-ff-ubsan/dist/bin/libxul.so+0x2144e9da)
    #39 0x7f2f2ec5ec85 in nsCommandLine::EnumerateHandlers(nsresult (*)(nsICommandLineHandler*, nsICommandLine*, void*), void*) toolkit/components/commandlines/nsCommandLine.cpp:448:10
    #40 0x7f2f2ec5f64f in nsCommandLine::Run() toolkit/components/commandlines/nsCommandLine.cpp:503:8
    #41 0x7f2f2f102d1d in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:4494:19
    #42 0x7f2f2f10427e in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) toolkit/xre/nsAppRunner.cpp:4701:8
    #43 0x7f2f2f1049b3 in XRE_main(int, char**, mozilla::BootstrapConfig const&) toolkit/xre/nsAppRunner.cpp:4752:21
    #44 0x56409f380da2 in do_main(int, char**, char**) browser/app/nsBrowserApp.cpp:217:22
    #45 0x56409f3804fb in main browser/app/nsBrowserApp.cpp:331:16

Comment 1

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/I_JdB1eh2O3iKVSUYHyyuA/index.html

Comment 2

[Emilio Cobos Álvarez (:emilio)]

This is intentional: https://searchfox.org/mozilla-central/rev/a1592902acabf9bded973067133baaac1457f3d3/mfbt/EnumeratedRange.h#189

Ok, so my understanding of C++ enum specese is wrong: https://stackoverflow.com/questions/18195312/what-happens-if-you-static-cast-invalid-value-to-enum-class

Apparently if you don't define a fixed representation (that is, if you don't define the underlying type of the enum), then outside-of-range values are invalid:

For an enumeration whose underlying type is fixed, the values of the enumeration are the values of the underlying type. Otherwise, for an enumeration where emin is the smallest enumerator and emax is the largest, the values of the enumeration are the values in the range bmin to bmax, defined as follows: Let K be 1 for a two's complement representation and 0 for a one's complement or sign-magnitude representation. bmax is the smallest value greater than or equal to max(|emin| − K, |emax|) and equal to 2M − 1, where M is a non-negative integer. bmin is zero if emin is non-negative and −(bmax + K) otherwise.

I think the fix for us may just be making this line be enum Side : uint32_t or such.

Does your reading match mine? This is a bit of a footgun, there's no way to static_assert that the underlying value is fixed in C++ :(

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1614512 - Give mozilla::Side and mozilla::Corner a fixed underlying type. r=TYLin,froydnj

So as to avoid UB. It is somewhat unfortunate/dumb the fact that we need to do
this and we can't detect when we forget to do it :(

Give it uint8_t as it's type as that's enough and consistent with LogicalSide.

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Re comment 2:

For an enumeration whose underlying type is fixed, the values of the enumeration are the values of the underlying type.

Yeah, it looks like we are required to define the underlying so that the usage of MakeInclusiveEnumeratedRange is valid.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7420023c50d3
Give mozilla::Side and mozilla::Corner a fixed underlying type. r=froydnj

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7420023c50d3

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Is there a real-world impact here which would make us want to consider uplifting or can this fix ride 75 to release?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Probably not in practice. Seems our compilers are fine doing the obvious thing here.