Closed Bug 1100338 Opened 5 years ago Closed 4 years ago

Uninitialised value use in nsUrlClassifierLookupCallback::Completion

Categories

(Toolkit :: Safe Browsing, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jseward, Assigned: gcp)

Details

Attachments

(1 file, 1 obsolete file)

I don't know whether this is real or a Valgrind bug, but anyway:

Unfortunately I don't have good STR, and repeating the problem is
tricky.  To reproduce, run http://gregor-wagner.com/tmp/mem50 and let
load the 50 sites.  When it produces a popup claiming it is finished,
switch to tabcandy (Panorama?) view, although I don't know if that is
actually necessary.  After a considerable amount of CPU time (because
V is so slow), in the range 30 to 60 CPU minutes, the error is
reported.

There is no origin data despite --track-origins=yes being present,
which has in the past been a sign of a Valgrind bug.

Conditional jump or move depends on uninitialised value(s)
   at 0x480C393: __memcmp_sse4_1 (/home/sewardj/VgTRUNK/trunk/memcheck/../shared/vg_replace_strmem.c:984)
   by 0x781AE0E: Compare (toolkit/components/url-classifier/Entries.h:133)
   by 0x781AE0E: operator== (toolkit/components/url-classifier/Entries.h:72)
   by 0x781AE0E: nsUrlClassifierLookupCallback::Completion(nsACString_internal const&, nsACString_internal const&, unsigned int) (toolkit/components/url-
classifier/nsUrlClassifierDBService.cpp:924)
   by 0x58A87DF: NS_InvokeByIndex (xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:164)
   by 0x5EB3621: Invoke (js/xpconnect/src/XPCWrappedNative.cpp:2394)
   by 0x5EB3621: Call (js/xpconnect/src/XPCWrappedNative.cpp:1746)
   by 0x5EB3621: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (js/xpconnect/src/XPCWrappedNative.cpp:1713)
   by 0x5EB4829: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1250)
   by 0x81EB636: CallJSNative (js/src/jscntxtinlines.h:231)
   by 0x81EB636: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (js/src/vm/Interpreter.cpp:482)
   by 0x81E2A6C: Interpret(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:2517)
   by 0x81EAE34: js::RunScript(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:432)
   by 0x81EB56E: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (js/src/vm/Interpreter.cpp:501)
   by 0x81EC1A2: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) (js/src/vm/Inte
rpreter.cpp:538)
   by 0x80FDF7E: JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>
) (js/src/jsapi.cpp:5001)
   by 0x5EB0D98: nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (js/xpconnect/src/XPCWr
appedJSClass.cpp:1187)
   by 0x58A9470: PrepareAndDispatch (xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp:122)
   by 0x58A896A: SharedStub (in /home/sewardj/MOZ/MC-08-11-2014/ff-O2-linux64/toolkit/library/libxul.so)
   by 0x5A5145E: mozilla::net::nsHttpChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) (netwerk/protocol/http/nsHttpChannel.cpp:5524)
   by 0x5917A1C: OnStateStop (netwerk/base/src/nsInputStreamPump.cpp:721)
   by 0x5917A1C: nsInputStreamPump::OnStateStop() (netwerk/base/src/nsInputStreamPump.cpp:676)

Conditional jump or move depends on uninitialised value(s)
   at 0x781AE11: nsUrlClassifierLookupCallback::Completion(nsACString_internal const&, nsACString_internal const&, unsigned int) (toolkit/components/url-
classifier/nsUrlClassifierDBService.cpp:924)
   by 0x58A87DF: NS_InvokeByIndex (xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:164)
   by 0x5EB3621: Invoke (js/xpconnect/src/XPCWrappedNative.cpp:2394)
   by 0x5EB3621: Call (js/xpconnect/src/XPCWrappedNative.cpp:1746)
   by 0x5EB3621: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (js/xpconnect/src/XPCWrappedNative.cpp:1713)
   by 0x5EB4829: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1250)
   by 0x81EB636: CallJSNative (js/src/jscntxtinlines.h:231)
   by 0x81EB636: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (js/src/vm/Interpreter.cpp:482)
   by 0x81E2A6C: Interpret(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:2517)
   by 0x81EAE34: js::RunScript(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:432)
   by 0x81EB56E: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (js/src/vm/Interpreter.cpp:501)
   by 0x81EC1A2: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) (js/src/vm/Interpreter.cpp:538)
   by 0x80FDF7E: JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) (js/src/jsapi.cpp:5001)
   by 0x5EB0D98: nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (js/xpconnect/src/XPCWrappedJSClass.cpp:1187)
   by 0x58A9470: PrepareAndDispatch (xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp:122)
   by 0x58A896A: SharedStub (in /home/sewardj/MOZ/MC-08-11-2014/ff-O2-linux64/toolkit/library/libxul.so)
   by 0x5A5145E: mozilla::net::nsHttpChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) (netwerk/protocol/http/nsHttpChannel.cpp:5524)
   by 0x5917A1C: OnStateStop (netwerk/base/src/nsInputStreamPump.cpp:721)
   by 0x5917A1C: nsInputStreamPump::OnStateStop() (netwerk/base/src/nsInputStreamPump.cpp:676)
   by 0x5917D2E: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (netwerk/base/src/nsInputStreamPump.cpp:440)
Exactly the same thing observed on MacOSX 10.10 64-bit.  This
increases the chances of the problem being real.

Conditional jump or move depends on uninitialised value(s)
   at 0x10B870F7A: nsUrlClassifierLookupCallback::Completion(nsACString_internal const&, nsACString_internal const&, unsigned int) (toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:924)
   by 0x1099F2C9C: NS_InvokeByIndex (xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:162)
   by 0x109EDB211: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (js/xpconnect/src/XPCWrappedNative.cpp:2395)
   by 0x109EDC85F: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1244)
   by 0x10C170F77: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (js/src/jscntxtinlines.h:231)
   by 0x10C18673E: Interpret(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:2526)
   by 0x10C17B976: js::RunScript(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:432)
   by 0x10C1711AF: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (js/src/vm/Interpreter.cpp:501)
What component should this be set to?  I couldn't figure that out, 
hence set it to Untriaged.
Flags: needinfo?(mmc)
The component for safebrowsing is either Toolkit::Phishing Protection or Firefox::DOM::Security, depending on who you believe. This if failing on the first line below. Looks like we are failing on the hash comparison from comment 0.

    // Now, see if it verifies a lookup
    if (result.CompleteHash() == hash && result.mTableName.Equals(tableName)) {
      result.mProtocolConfirmed = true;
    }
  }
Flags: needinfo?(mmc)
Product: Core → Firefox
Component: Untriaged → DOM: Security
Product: Firefox → Core
  mozilla::safebrowsing::Completion hash;
  hash.Assign(completeHash);

    if (result.CompleteHash() == hash && result.mTableName.Equals(tableName)) {


.Assign seems to do the right thing, so it must be result (LookupResult) that's uninitialized.
Probably due to http://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/LookupCache.h#29
result contains a Prefix() so trying to compare vs a Completion() ends up comparing against uninitialized RAM.

Probably this needs a check for mComplete somewhere but the fact that we have this bug means it's a good idea to clean up some stuff.
gcp, any possibility to progress this?  It's still alive and triggers
after not very much real surfing.
Attached patch A possible fix (obsolete) — Splinter Review
This stops Valgrind complaining by adding a check for mComplete at one
place where it seems to be missing.  In other words, if I understand
the code correctly, it avoids comparing a Prefix against a Completion.
Attachment #8629906 - Flags: review?(gpascutto)
Comment on attachment 8629906 [details] [diff] [review]
A possible fix

Review of attachment 8629906 [details] [diff] [review]:
-----------------------------------------------------------------

mResults is set here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#790

It seems to initially contain the array of Prefixes, which are then sent to the completer via LookupComplete and will end up as completions. You hitting this implies you can end up in Completion when there's still prefix-only entries in mResults. I guess this may be the expected case on a page that has multiple hits. In any case although this seemed worrying at first I can't point out anything wrong.
Attachment #8629906 - Flags: review?(gpascutto) → review+
(In reply to Julian Seward [:jseward] from comment #7)
> Created attachment 8629906 [details] [diff] [review]
> A possible fix

Unfortunately this causes the xpcshell test
toolkit/components/url-classifier/tests/unit/test_partial.js to fail,
where it succeeded before.  I had a bit of a look but I am completely
lost.  Any suggestions?
Flags: needinfo?(gpascutto)
Debug it?
Flags: needinfo?(gpascutto)
Sorry for being short there but if this problem was obvious I'd just have patched it when you originally reported it. If you need me to fix it urgently, assign it to me. But this needinfoing is pointless.
Uninitialized value errors are bad and worth fixing.
Assignee: nobody → gpascutto
Ping.  Any possibly to advance this?  It is still alive.
I've been looking at this again.

mComplete has no bearing on whether the LookupResult has a Complete hash or a Prefix: things are added to LookupResult if there is a hit, and mComplete indicates what the hit was against, not whether the hash itself in LookupResult is complete. The actual LookupResult.hash should always be complete, it's directly initialized as such in Classifier::Check from a Completion.

The reason why Valgrind passes with the attached patch but tests fail is that the mComplete check is false in both tests and (most) normal situations, so we don't see the uninitialized memory but the code doesn't ever actually do anything either.

So I was at loss how there can be LookupResults with uninitialized parts. I didn't find any points where its concurrently modified (though this is messy). The other part of the failing == is set using Completion.Assign which contains an assertion to make sure it's the right size.

However, I think I see the issue now. AddNoise is modifying the LookupResultArray and initializing hash.prefix. So that part isn't using Completes. It would also explain why this doesn't trigger in tests, only in "real" usage.
Attachment #8688471 - Flags: review?(francois)
Attachment #8688471 - Flags: feedback?(jseward)
Attachment #8629906 - Attachment is obsolete: true
Attachment #8688471 - Flags: review?(francois) → review+
Component: DOM: Security → Safe Browsing
Product: Core → Toolkit
Comment on attachment 8688471 [details] [diff] [review]
Do not consider noise for Completion matches. Remove dead code

With the patch applied I can see no more uninitialised value uses.
So, looks good to me.
Attachment #8688471 - Flags: feedback?(jseward) → feedback+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e4218058d633480b4a17db6cf3d3831418b6ec3
Bug 1100338 - Do not consider noise for Completion matches. Remove dead code. r=francois
https://hg.mozilla.org/mozilla-central/rev/9e4218058d63
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.