Closed Bug 1625161 Opened 4 years ago Closed 2 years ago

Move ASSERT_UNLESS_FUZZING to mfbt

Categories

(Core :: MFBT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
97 Branch
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.

Flags: needinfo?(twsmith)

Sure. But what changes #define DISABLE_ASSERTS_FOR_FUZZING 0 ? It looks like something you have to manually edit in your local tree.

I don't believe we use it anymore. I don't know the history behind this but decoder does.

Let's remove them and see what happens?

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?

Flags: needinfo?(twsmith) → needinfo?(nika)

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.)

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

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.

Flags: needinfo?(nika)
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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 97 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: