Open Bug 1467468 Opened 2 years ago Updated 2 years ago

Faulty: Run ReadFile() for IsMessageNameBlacklisted() on the main thread

Categories

(Core :: Platform Fuzzing Team, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

REOPENED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: posidron, Assigned: posidron)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

Attached patch faulty.readfile.omt.diff (obsolete) — Splinter Review
This patch does fix an assertion failure crash caused by a check for NS_IsMainThread() in nsStandardURL.cpp introduced by https://bugzilla.mozilla.org/show_bug.cgi?id=1447190

Assertion failure: NS_IsMainThread(), at /builds/worker/workspace/build/src/netwerk/base/nsStandardURL.cpp:235
#01: nsNetStartup() /builds/worker/workspace/build/src/netwerk/build/nsNetModule.cpp:630:5
#02: Load /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:726:21
#03: nsFactoryEntry::GetFactory() /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:1748:0
#04: nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp:1046:41
#05: nsCreateInstanceByContractID::operator()(nsID const&, void**) const /builds/worker/workspace/build/src/xpcom/components/nsComponentManagerUtils.cpp:198:7
#06: nsCOMPtr_base::assign_from_helper(nsCOMPtr_helper const&, nsID const&) /builds/worker/workspace/build/src/xpcom/base/nsCOMPtr.cpp:128:7
#07: mozilla::ipc::Faulty::ReadFile(char const*, nsTArray<nsTString<char> >&) /builds/worker/workspace/build/src/tools/fuzzing/faulty/Faulty.cpp:720:32
#08: mozilla::ipc::Faulty::IsMessageNameBlacklisted(char const*) /builds/worker/workspace/build/src/tools/fuzzing/faulty/Faulty.cpp:761:53
#09: mozilla::ipc::Faulty::MutateIPCMessage(char const*, IPC::Message*, unsigned int) /builds/worker/workspace/build/src/tools/fuzzing/faulty/Faulty.cpp:818:32
#10: IPC::Channel::ChannelImpl::Send(IPC::Message*) /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc:775:53
#11: mozilla::detail::RunnableMethodImpl<IPC::Channel*, bool (IPC::Channel::*)(IPC::Message*), false, (mozilla::RunnableKind)0, IPC::Message*>::Run() /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1219:5
#12: assign_assuming_AddRef /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:354:9
#13: operator= /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:638:0
#14: RunTask /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:453:0
#15: DeferOrRunPendingTask /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:460:0
#16: MessageLoop::DoWork() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:535:0
#17: base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/chromium/src/base/message_pump_libevent.cc:352:31
#18: ~AutoRunState /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:599:19
#19: MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:300:0
#20: base::Thread::ThreadMain() /builds/worker/workspace/build/src/ipc/chromium/src/base/thread.cc:184:3
#21: ThreadFunc(void*) /builds/worker/workspace/build/src/ipc/chromium/src/base/platform_thread_posix.cc:39:3
#22: start_thread ??:0:0
#23: clone /build/glibc-itYbWN/glibc-2.26/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:97:0
Attachment #8984142 - Flags: superreview?(jld)
Attachment #8984142 - Flags: review?(valentin.gosu)
Attached patch faulty.readfile.omt.diff (obsolete) — Splinter Review
Attachment #8984142 - Attachment is obsolete: true
Attachment #8984142 - Flags: superreview?(jld)
Attachment #8984142 - Flags: review?(valentin.gosu)
Attachment #8984145 - Flags: superreview?(jld)
Attachment #8984145 - Flags: review?(valentin.gosu)
Comment on attachment 8984142 [details] [diff] [review]
faulty.readfile.omt.diff

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

nit: I don't think you need to include the stack trace in the commit message. If anyone is interested in it, they can find it in this bug.

::: tools/fuzzing/faulty/Faulty.cpp
@@ +763,3 @@
>    static nsTArray<nsCString> sMessageBlacklist;
>    if (!sFileLoaded && mBlacklistPath) {
> +    // Run ReadFile() on the main thread to prevent NS_IsMainThread() in nsStandardURL.

... to prevent MOZ_ASSERT(NS_IsMainThread()) in nsStandardURL via nsNetStartup()

@@ +764,5 @@
>    if (!sFileLoaded && mBlacklistPath) {
> +    // Run ReadFile() on the main thread to prevent NS_IsMainThread() in nsStandardURL.
> +    nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
> +       "Fuzzer::ReadBlacklistOnMainThread",
> +        [&]() {

nit: Line above has 3 chars indent, this one 4. Let's indent everything with 2 spaces :)

@@ +776,4 @@
>    }
> +  if (!sFileLoaded) {
> +   return false;
> +   }

nit: bad 1 space indentation here.
Attachment #8984142 - Attachment is obsolete: false
Comment on attachment 8984145 [details] [diff] [review]
faulty.readfile.omt.diff

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

Fixing review comments before they are posted is a cool superpower :)
Attachment #8984145 - Flags: review?(valentin.gosu) → review+
Blocks: 1447425
Attachment #8984145 - Flags: superreview?(nfroyd)
Attachment #8984145 - Flags: superreview?(jld)
Attachment #8984145 - Flags: review+
Jed seems to be too busy with other work, hence am redirecting the super-review to Nathan.
Comment on attachment 8984145 [details] [diff] [review]
faulty.readfile.omt.diff

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

r=me, thanks!
Attachment #8984145 - Flags: superreview?(nfroyd) → superreview+
Keywords: checkin-needed
Attachment #8984142 - Attachment is obsolete: true
Could you please update the commit message for the appropriate one so we could land this patch?
Flags: needinfo?(nfroyd)
Comment on attachment 8984145 [details] [diff] [review]
faulty.readfile.omt.diff

This is Christoph's patch.

OTOH, How do we know this always gets called off the main thread?  Shouldn't you really be checking that you're on the main thread, and either reading the file straightaway if you are, and dispatching a runnable if you're not?  I guess the code works because of the way NS_DISPATCH_SYNC works...
Flags: needinfo?(nfroyd) → needinfo?(cdiehl)
Assignee: nobody → cdiehl
Keywords: checkin-needed
Attachment #8984145 - Attachment is obsolete: true
Flags: needinfo?(cdiehl)
I need to get this checked-in as long as I have a chance to talk to roc at the AllHands.
Keywords: checkin-needed
Attachment #8985406 - Attachment is patch: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7bf77f5b69a
Faulty: Run ReadFile() for IsMessageNameBlacklisted() on the main thread. r=valentin, sr=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f7bf77f5b69a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(In reply to Nathan Froyd [:froydnj] from comment #7)
> OTOH, How do we know this always gets called off the main thread?  Shouldn't
> you really be checking that you're on the main thread, and either reading
> the file straightaway if you are, and dispatching a runnable if you're not? 
> I guess the code works because of the way NS_DISPATCH_SYNC works...

So I tried that but it seems there is a difference between Linux and MacOS.

I tested the following with two scenarios:

if (NS_IsMainThread()) { 
  ReadFile(); ...
}
else { 
  NS_NewRunnableFunction([&]() { ReadFile(); ... } ); 
  NS_DispatchToMainThread(r.forget(), NS_DISPATCH_SYNC); 
}


1) running with ./mach crashtest
2) running with the Firefox binary standalone and pointing it to a random HTML crashtest

* In both scenarios. If IsMainThread=false and we spawn a new runnable, the process stalls (from time to time). Though, this happens only on Linux but not on MacOS.

* If we only check for IsMainThread(true) the ReadFile() routine is never called, so we are always not on the main thread while calling ReadFile().

* If we simply call ReadFile() we will get the Assertion failure from above that we are not on the main thread.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file threads-bt.txt
You need to log in before you can comment on or make changes to this bug.