Move ASSERT_UNLESS_FUZZING to mfbt
Categories
(Core :: MFBT, enhancement)
Tracking
()
Tracking | Status | |
---|---|---|
firefox97 | --- | fixed |
People
(Reporter: jesup, Assigned: jesup)
References
Details
Attachments
(1 file)
ContentParent has ASSERT_UNLESS_FUZZING; this really should be in mfbt (per the comment there).
We could rename it to MOZ_ASSERT_UNLESS_FUZZING for consistency if we like
Tyson, are these controls actually used by the fuzzing team? This code (especially the fact that multiple cpp files control #define DISABLE_ASSERTS_FOR_FUZZING
independently!) really smells of the old pattern where people would have personal ifdefs for local testing. IMO we shouldn't have things like that in the tree anymore -- we could have a formal configure-level switch for these things if needed, but if we're not even using these controls, I think we should just enable the asserts unconditionally.
Comment 2•4 years ago
|
||
Sure. But what changes #define DISABLE_ASSERTS_FOR_FUZZING 0
? It looks like something you have to manually edit in your local tree.
Comment 4•4 years ago
|
||
I don't believe we use it anymore. I don't know the history behind this but decoder does.
Comment 5•4 years ago
|
||
Let's remove them and see what happens?
Comment 6•4 years ago
|
||
I've been looking cursorily at some of these asserts and it looks like these might be required to disable if we want to run e.g. Faulty. Some of these asserts protect invariants that we would expect from a "properly behaving" child, but with Faulty and other IPC fuzzing, the child could easily violate these.
Removing these is not a good idea, we should instead tie them into the FUZZING
definition. Nika, do you have any additional thoughts on these?
For clarity, the proposal is not to remove the asserts, but to make them assert unconditionally. (I'm pretty sure we all agree on this but might have been typing in a hurry.)
Comment 8•4 years ago
|
||
(In reply to :dmajor from comment #7)
For clarity, the proposal is not to remove the asserts, but to make them assert unconditionally. (I'm pretty sure we all agree on this but might have been typing in a hurry.)
Making them assert unconditionally is not the right approach either. Some of these asserts are expected to be violated during fuzzing, they are only useful if you assume the child is behaving correctly.
Comment 9•4 years ago
|
||
DISABLE_ASSERTS_FOR_FUZZING
definitely seems like an old pattern which isn't used anymore. I think it makes sense to have centrally-defined MOZ_ASSERT_UNLESS_FUZZING
macros which are controlled by the FUZZING
define, as that will give us better tools for relaxing assertions which can be violated by a poorly behaved child process.
I think I agree with :decoder that we shouldn't make asserts which validate a well-behaving content process unconditional, as that hampers fuzzing our IPC layer, while having these assertions in our debug builds can be useful for catching bugs.
Assignee | ||
Comment 10•2 years ago
|
||
Comment 11•2 years ago
|
||
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/b0f9cf3e8216 Move ASSERT_UNLESS_FUZZING() to mfbt and hook into FUZZING r=glandium,decoder
Comment 12•2 years ago
|
||
bugherder |
Description
•