1100338 - Uninitialised value use in nsUrlClassifierLookupCallback::Completion

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Julian Seward [:jseward]]

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)

Comment 1

[Julian Seward [:jseward]]

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)

Comment 2

[Julian Seward [:jseward]]

What component should this be set to?  I couldn't figure that out, 
hence set it to Untriaged.

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

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;
    }
  }

Comment 4

[Gian-Carlo Pascutto [:gcp]]

  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.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 6

[Julian Seward [:jseward]]

gcp, any possibility to progress this?  It's still alive and triggers
after not very much real surfing.

Comment 7

[Julian Seward [:jseward]]

Attachment: A possible fix

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.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 9

[Julian Seward [:jseward]]

(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?

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Debug it?

Comment 11

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 12

[Nicholas Nethercote [inactive]]

Uninitialized value errors are bad and worth fixing.

Comment 13

[Julian Seward [:jseward]]

Ping.  Any possibly to advance this?  It is still alive.

Comment 14

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 15

[Gian-Carlo Pascutto [:gcp]]

Attachment: Do not consider noise for Completion matches. Remove dead code

Comment 16

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f26f692e1eb0

Comment 17

[Julian Seward [:jseward]]

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.

Comment 18

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e4218058d633480b4a17db6cf3d3831418b6ec3
Bug 1100338 - Do not consider noise for Completion matches. Remove dead code. r=francois

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e4218058d63