Closed Bug 1431439 Opened 2 years ago Closed 2 years ago
Land JS fuzzing target for Structured
This bug is about landing a fuzzing target for StructuredCloneReader once bug 1431090 landed. We need to make sure to extensively test this before landing in order to not accidentally expose a bug.
This patch adds the fuzzing target for StructuredCloneReader, but also changes the way we add libfuzzer's instrumentation flags to the target source code. Local experiments have shown that adding the flags to all of the JS engine (like I implemented it in bug 1431090) causes quite some noise. Instead we should only build the files that we are actually targeting with those flags. This requires taking them out of UNIFIED_SOURCES when a fuzzing build with libfuzzer is requested.
Attachment #8947083 - Flags: review?(sphink)
Comment on attachment 8947083 [details] [diff] [review] fuzzing-target-sc.patch Review of attachment 8947083 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/fuzz-tests/testStructuredCloneReader.cpp @@ +8,5 @@ > +#include "mozilla/ScopeExit.h" > +#include "jsapi.h" > +#include "fuzz-tests/tests.h" > +#include "vm/Interpreter.h" > +#include "jscntxtinlines.h" I think our style checker is going to scream at you here. I really should review jorendorff's patch to add a --fixup option to just reorder the includes for you. @@ +13,5 @@ > + > +using namespace JS; > +using namespace js; > + > +extern JS::PersistentRootedObject gGlobal; Pick one way or the other -- either say 'using namespace JS' and leave off the JS:: qualifier, or qualify everywhere. (Or do 'using' in a non-toplevel scope and qualify at the top.) @@ +15,5 @@ > +using namespace js; > + > +extern JS::PersistentRootedObject gGlobal; > +extern JSContext* gCx; > + Can you comment where these two are actually defined? @@ +33,5 @@ > + > + // Make sure to pad the buffer to a multiple of kSegmentAlignment > + const size_t kSegmentAlignment = 8; > + size_t size_diff = kSegmentAlignment - (size % kSegmentAlignment); > + size_t buf_size = size + size_diff; Probably easier to read as size_t buf_size = JS_ROUNDUP(size, kSegmentAlignment); not to mention that this is always padding (so it adds 8 bytes to an already-aligned size). Unless you want that for some reason.
Attachment #8947083 - Flags: review?(sphink) → review+
Updated patch to address review comments. Green on try, will land as soon as bug 1434384 landed because that is the only remaining unfixed security bug that could be easily found with this patch.
Rebased patch plus some cleanups with flags. Cleanup parts reviewed by :jandem over IRC.
IIRC, we fuzz the release branches as well. Is this worth a backport to Beta60?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #7) > IIRC, we fuzz the release branches as well. Is this worth a backport to > Beta60? We only do so in exceptional cases right now, so no. I would let this ride the trains.
Whiteboard: [adv-main61-] → [adv-main61-][post-critsmash-triage]
You need to log in before you can comment on or make changes to this bug.