Closed Bug 1436156 Opened 2 years ago Closed 2 years ago

CHECK() in Chromium IPC code should be fatal when not fuzzing

Categories

(Core :: IPC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jld, Assigned: jld)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 777067 changed the CHECK() macro in Chromium-derived IPC code, and by extension other variants like DCHECK(), from a fatal assertion to a warning.  This is not conditional on fuzzing; it applies to all builds, all the time.

We really shouldn't be skipping assertions like that outside of fuzz testing; I'd been assuming that I could depend on assertions like [1] to make problems immediately obvious, but now they log one message (easily missed in all the usual stderr junk) and proceed to do bad things.

Fortunately, there don't appear to be any security-sensitive uses of CHECK() at present, but it really doesn't seem like a good idea to have something that looks like the equivalent of MOZ_RELEASE_ASSERT but is basically NS_WARN_IF.


[1] https://searchfox.org/mozilla-central/rev/f80722d4f3bfb722c5ec53880c4a7efb71285676/ipc/chromium/src/base/file_descriptor_shuffle.cc#28
Priority: -- → P2
Assignee: nobody → cdiehl
Attachment #8956289 - Flags: review?(jld)
Comment on attachment 8956289 [details] [diff] [review]
faulty.check.diff

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

Thanks for the patch.
Attachment #8956289 - Flags: review?(jld) → review+
Pushed by toros@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aef35642e5f
CHECK() in Chromium IPC code should be fatal when not fuzzing. r=jld
Keywords: checkin-needed
Backed out for

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/406e69f3fd2e8ac5c2254556a72332a02222723b

push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7aef35642e5facf784d9a41b96e54cf35f6fb5a3

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=166178145&repo=mozilla-inbound&lineNumber=9716

[task 2018-03-06T11:29:51.389Z] 11:29:51     INFO - TEST-START | dom/indexedDB/test/browser_permissionsPromptAllow.js
[task 2018-03-06T11:29:51.434Z] 11:29:51     INFO - GECKO(3378) | ++DOCSHELL 0xe37a8800 == 3 [pid = 3495] [id = {8b344ef0-1ddc-44a8-bcde-a9274a4eb8b9}]
[task 2018-03-06T11:29:51.435Z] 11:29:51     INFO - GECKO(3378) | ++DOMWINDOW == 7 (0xf714b480) [pid = 3495] [serial = 7] [outer = (nil)]
[task 2018-03-06T11:29:51.538Z] 11:29:51     INFO - GECKO(3378) | ++DOMWINDOW == 8 (0xe37aa400) [pid = 3495] [serial = 8] [outer = 0xf714b480]
[task 2018-03-06T11:29:52.024Z] 11:29:52     INFO - GECKO(3378) | ++DOMWINDOW == 9 (0xe37a9800) [pid = 3495] [serial = 9] [outer = 0xf714b480]
[task 2018-03-06T11:29:52.329Z] 11:29:52     INFO - GECKO(3378) | [Child 3495, Main Thread] ###!!! ABORT: Inserting duplicate item: file /builds/worker/workspace/build/src/ipc/chromium/src/base/id_map.h, line 60
[task 2018-03-06T11:29:52.329Z] 11:29:52     INFO - GECKO(3378) | #01: mozilla::Logger::~Logger [ipc/chromium/src/base/logging.cc:48]
[task 2018-03-06T11:29:52.329Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.331Z] 11:29:52     INFO - GECKO(3378) | #02: IDMap<nsCOMPtr<nsIEventTarget> >::AddWithID [ipc/chromium/src/base/logging.h:62]
[task 2018-03-06T11:29:52.332Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.333Z] 11:29:52     INFO - GECKO(3378) | #03: mozilla::ipc::IToplevelProtocol::SetEventTargetForActorInternal [ipc/glue/ProtocolUtils.cpp:888]
[task 2018-03-06T11:29:52.334Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.336Z] 11:29:52     INFO - GECKO(3378) | #04: mozilla::dom::indexedDB::BackgroundFactoryRequestChild::RecvPermissionChallenge [dom/indexedDB/ActorsChild.cpp:2001]
[task 2018-03-06T11:29:52.338Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.339Z] 11:29:52     INFO - GECKO(3378) | #05: mozilla::dom::indexedDB::PBackgroundIDBFactoryRequestChild::OnMessageReceived [s3:gecko-generated-sources:cd14fd58c8fba1730941f9292daee57b67028ebc266682bb0f9dc82830a0263979b766bf0f31a72bef0087ae521bf018d18cae3d6d98a3ddcbca39a2560d6451/ipc/ipdl/PBackgroundIDBFactoryRequestChild.cpp::145]
[task 2018-03-06T11:29:52.341Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.342Z] 11:29:52     INFO - GECKO(3378) | #06: mozilla::ipc::MessageChannel::DispatchAsyncMessage [ipc/glue/MessageChannel.h:664]
[task 2018-03-06T11:29:52.343Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.344Z] 11:29:52     INFO - GECKO(3378) | #07: mozilla::ipc::MessageChannel::DispatchMessage [ipc/glue/MessageChannel.cpp:2065]
[task 2018-03-06T11:29:52.345Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.347Z] 11:29:52     INFO - GECKO(3378) | #08: mozilla::ipc::MessageChannel::RunMessage [ipc/glue/MessageChannel.cpp:1909]
[task 2018-03-06T11:29:52.354Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.356Z] 11:29:52     INFO - GECKO(3378) | #09: mozilla::ipc::MessageChannel::MessageTask::Run [ipc/glue/MessageChannel.cpp:1943]
[task 2018-03-06T11:29:52.357Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.359Z] 11:29:52     INFO - GECKO(3378) | #10: mozilla::SchedulerGroup::Runnable::Run [xpcom/threads/SchedulerGroup.cpp:413]
[task 2018-03-06T11:29:52.360Z] 11:29:52     INFO - 
[task 2018-03-06T11:29:52.364Z] 11:29:52     INFO - GECKO(3378) | #11: nsThread::ProcessNextEvent [mfbt/Maybe.h:100]
Flags: needinfo?(cdiehl)
Backed out for failing ipc/chromium/src/base/id_map.h:60 ^
That looks related to bug 1317844.  I'll investigate.
Flags: needinfo?(cdiehl) → needinfo?(jld)
Flags: needinfo?(jld)
OS: Unspecified → All
Hardware: Unspecified → All
I'm just going to put in a workaround for bug 1445121 to get this landed.
Assignee: cdiehl → jld
Priority: P2 → P1
(If there's another IPC reviewer who knows more about event targets, feel free to redirect the request.)
Comment on attachment 8967215 [details]
Bug 1436156 - CHECK() in Chromium IPC code should be fatal when not fuzzing.

https://reviewboard.mozilla.org/r/235886/#review241788

I can't decide whether it says good things about our IPC code that we don't have to fix up more things after tweaking this.

::: ipc/chromium/src/base/logging.h:122
(Diff revision 1)
> +#ifdef FUZZING
>  #define CHECK(condition) LOG_IF(WARNING, condition)

Asking because I don't know: do the fuzzing people comb through the logs afterward to find messages like this and see if they are sensical?  Or is the desire to avoid the random fuzz data crashing the browser in weird places, and botching up the fuzzing runs?
Attachment #8967215 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #11)
> Asking because I don't know: do the fuzzing people comb through the logs
> afterward to find messages like this and see if they are sensical?  Or is
> the desire to avoid the random fuzz data crashing the browser in weird
> places, and botching up the fuzzing runs?

The latter.
Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b413e24051f
CHECK() in Chromium IPC code should be fatal when not fuzzing. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/8b413e24051f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
See Also: → 1454132
You need to log in before you can comment on or make changes to this bug.