Closed Bug 1436240 Opened 2 years ago Closed 2 years ago

UBSan: load of value which is not a valid value for type 'bool' in /layout/style/MediaQueryList.cpp:78

Categories

(Core :: Layout, defect, P3)

60 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: tsmith, Assigned: emilio)

Details

(Keywords: csectype-undefined)

Attachments

(1 file)

This seems to be triggered after a few minutes with regular browsing.

Found in mozilla-central changeset: 402372:3df7961bad2c. Built with -fsanitize=bool

I am guessing this is actually uninitialized memory but I'm not 100%.

/layout/style/MediaQueryList.cpp:78:10: runtime error: load of value 228, which is not a valid value for type 'bool'
    #0 0x7fa22e2ef382 in mozilla::dom::MediaQueryList::Matches() /layout/style/MediaQueryList.cpp:78:10
    #1 0x7fa22aa4686c in mozilla::dom::MediaQueryListBinding::get_matches(JSContext*, JS::Handle<JSObject*>, mozilla::dom::MediaQueryList*, JSJitGetterCallArgs) /objdir-ff-ubsan/dom/bindings/MediaQueryListBinding.cpp:66:21
    #2 0x7fa22bfe64aa in mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) /dom/bindings/BindingUtils.cpp:2906:13
    #3 0x7fa23446a88e in CallJSNative /js/src/jscntxtinlines.h:291:15
    #4 0x7fa23446a88e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /js/src/vm/Interpreter.cpp:473
    #5 0x7fa23446b4cf in InternalCall(JSContext*, js::AnyInvokeArgs const&) /js/src/vm/Interpreter.cpp:522:12
    #6 0x7fa23446b6aa in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /js/src/vm/Interpreter.cpp:541:10
    #7 0x7fa23446c898 in js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /js/src/vm/Interpreter.cpp:656:12
    #8 0x7fa2353fc5f5 in CallGetter(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<js::Shape*>, JS::MutableHandle<JS::Value>) /js/src/vm/NativeObject.cpp:2145:16
    #9 0x7fa2353dc4c1 in GetExistingProperty<js::AllowGC::CanGC> /js/src/vm/NativeObject.cpp:2198:12
    #10 0x7fa2353dc4c1 in NativeGetPropertyInline<js::AllowGC::CanGC> /js/src/vm/NativeObject.cpp:2401
    #11 0x7fa2353dc4c1 in js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /js/src/vm/NativeObject.cpp:2437
    #12 0x7fa2343d2eb4 in js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /js/src/vm/NativeObject.h:1630:12
    #13 0x7fa2343d2cc7 in js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, js::PropertyName*, JS::MutableHandle<JS::Value>) /js/src/jsobj.h:822:12
    #14 0x7fa234472593 in js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) /js/src/vm/Interpreter.cpp:4405:12
    #15 0x7fa234482647 in GetPropertyOperation(JSContext*, js::InterpreterFrame*, JS::Handle<JSScript*>, unsigned char*, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>) /js/src/vm/Interpreter.cpp:219:12
    #16 0x7fa23443f114 in Interpret(JSContext*, js::RunState&) /js/src/vm/Interpreter.cpp:2815:10
    #17 0x7fa234433428 in js::RunScript(JSContext*, js::RunState&) /js/src/vm/Interpreter.cpp:423:12
    #18 0x7fa23446a779 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /js/src/vm/Interpreter.cpp:495:15
    #19 0x7fa23446b4cf in InternalCall(JSContext*, js::AnyInvokeArgs const&) /js/src/vm/Interpreter.cpp:522:12
    #20 0x7fa23446b6aa in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /js/src/vm/Interpreter.cpp:541:10
    #21 0x7fa234d5f64e in js::jit::InvokeFunction(JSContext*, JS::Handle<JSObject*>, bool, bool, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /js/src/jit/VMFunctions.cpp:112:12
    #22 0x7fa234d60125 in js::jit::InvokeFromInterpreterStub(JSContext*, js::jit::InterpreterStubExitFrameLayout*) /js/src/jit/VMFunctions.cpp:141:10
    #23 0x7fa1c8b5df76  (<unknown module>)
Comment on attachment 8948878 [details]
Bug 1436240: Make sure to always initialize mMatches to something meaningful.

https://reviewboard.mozilla.org/r/218272/#review224060

May make more sense to have it be `Maybe<bool>`, but keeping them separate is probably fine as well, especially given there is code depending on their being separate.
Attachment #8948878 - Flags: review?(xidorn+moz) → review+
But actually, the question is why would this happen at all? The value is protected by mMatchesValid, so we shouldn't be reading mMatches as bool when mMatchesValid is false.
Assignee: nobody → emilio
Priority: -- → P3
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #2)
> Comment on attachment 8948878 [details]
> Bug 1436240: Make sure to always initialize mMatches to something meaningful.
> 
> https://reviewboard.mozilla.org/r/218272/#review224060
> 
> May make more sense to have it be `Maybe<bool>`, but keeping them separate
> is probably fine as well, especially given there is code depending on their
> being separate.

Yeah, maybe... I went for the cheap fix here I admit :)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> But actually, the question is why would this happen at all? The value is
> protected by mMatchesValid, so we shouldn't be reading mMatches as bool when
> mMatchesValid is false.

The key is that RecomputeMatches bails out in a bunch of places without setting mMatches, then don't bother checking, like in ::Matches()
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d71fadfe8e78
Make sure to always initialize mMatches to something meaningful. r=xidorn
(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> > But actually, the question is why would this happen at all? The value is
> > protected by mMatchesValid, so we shouldn't be reading mMatches as bool when
> > mMatchesValid is false.
> 
> The key is that RecomputeMatches bails out in a bunch of places without
> setting mMatches, then don't bother checking, like in ::Matches()

Oh, hmmm, that makes sense then.
https://hg.mozilla.org/mozilla-central/rev/d71fadfe8e78
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.