1538577 - Conditional jump or move depends on uninitialised value(s) [@ mozilla::dom::HTMLInputElement::ParseTime]

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found with m-c:
BuildID=20190319215514
SourceStamp=d15d511d7fed036d69d37cecbacdea779a983a34

==50640== Conditional jump or move depends on uninitialised value(s)
==50640==    at 0x119AFF3D: mozilla::dom::HTMLInputElement::ParseTime(nsTSubstring<char16_t> const&, unsigned int*) (HTMLInputElement.cpp:5035)
==50640==    by 0x119AE3D1: mozilla::dom::HTMLInputElement::SanitizeValue(nsTSubstring<char16_t>&) (HTMLInputElement.cpp:5012)
==50640==    by 0x119AF222: mozilla::dom::HTMLInputElement::SetValueInternal(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const*, unsigned int) (HTMLInputElement.cpp:2592)
==50640==    by 0x119A86FC: mozilla::dom::HTMLInputElement::SetValue(nsTSubstring<char16_t> const&, mozilla::dom::CallerType, mozilla::ErrorResult&) (HTMLInputElement.cpp:1562)
==50640==    by 0x115E3F76: mozilla::dom::HTMLInputElement_Binding::set_value(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLInputElement*, JSJitSetterCallArgs) (HTMLInputElementBinding.cpp:3006)
==50640==    by 0x116BEB51: bool mozilla::dom::binding_detail::GenericSetter<mozilla::dom::binding_detail::NormalThisPolicy>(JSContext*, unsigned int, JS::Value*) (BindingUtils.cpp:3097)
==50640==    by 0x13441E60: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:442)
==50640==    by 0x13443107: js::CallSetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) (Interpreter.cpp:589)
==50640==    by 0x1361D1C5: SetExistingProperty(JSContext*, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyResult>, JS::ObjectOpResult&) (NativeObject.cpp:2939)
==50640==    by 0x1361A44C: bool js::NativeSetProperty<(js::QualifiedBool)1>(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) (NativeObject.cpp:2968)
==50640==    by 0x13438DAF: Interpret(JSContext*, js::RunState&) (ObjectOperations-inl.h:283)
==50640==    by 0x1343078C: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:422)
==50640==  Uninitialised value was created by a stack allocation
==50640==    at 0x119AFED0: mozilla::dom::HTMLInputElement::ParseTime(nsTSubstring<char16_t> const&, unsigned int*) (HTMLInputElement.cpp:5016)
==50640== 

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

this looks like a false positive, or am I missing something.
https://searchfox.org/mozilla-central/rev/56705678f5fc363be5e0237e1686f619b0d23009/dom/html/HTMLInputElement.cpp#5034-5035
hours is used only if DigitSubStringToNumber returns true, and it returns true
only if the outparam is initialized
https://searchfox.org/mozilla-central/rev/56705678f5fc363be5e0237e1686f619b0d23009/dom/html/HTMLInputElement.cpp#5005,5008

Comment 2

[Tyson Smith [:tsmith]]

I agree this does look like a false positive.

Julian, I was using Valgrind (commit 7e9113cb7a249e0fae2a365462c6b0164de11566) with a build from TC[1] if you are interested.

[1] https://tools.taskcluster.net/index/gecko.v2.mozilla-central.latest.firefox/linux64-valgrind-opt

Comment 3

[Julian Seward [:jseward]]

Yes, almost certainly a false positive. Most likely the compiler
has swapped the order of evaluation of the arguments to ||, so that

  uint32_t hours;
  if (!DigitSubStringToNumber(aValue, 0, 2, &hours) || hours > 23) {

is compiled like this

  uint32_t hours;
  if (hours > 23 || !DigitSubStringToNumber(aValue, 0, 2, &hours)) {

which (amazingly) is a valid transformation if the compiler can prove
that !DigitSubStringToNumber(aValue, 0, 2, &hours) is true whenever
hours is undefined.

A probable workaround is to initialise hours to zero. That's one
extra insn on x86 targets.

Comment 4

[Julian Seward [:jseward]]

What I meant (as the result of the transformation) was

  uint32_t hours;
  bool b = !DigitSubStringToNumber(aValue, 0, 2, &hours);
  if (hours > 23 || b) {