Closed Bug 1405717 Opened 7 years ago Closed 7 years ago

Check if wrapWithProto can be made fuzzing-safe

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: decoder, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Keywords: sec-want, Whiteboard: [js:tech-debt][js:testability][adv-main58-])

Attachments

(1 file)

Apparently the testing function wrapWithProto is capable of covering some code in  js/src/proxy/Proxy.cpp that we cover with tests but not in fuzzing. The code in question is this:

http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/js/src/proxy/Proxy.cpp#340


We have e.g. js/src/jit-test/tests/basic/bug842940.js that uses this and hits the mentioned code location. It seems that this wasn't always fuzzing-unsafe because bug 842940 itself is a fuzzing bug.

NI for jandem based on IRC conversation.
Flags: needinfo?(jdemooij)
Priority: -- → P3
Whiteboard: [js:tech-debt][js:testability]
So bug 1126105 made this fuzzing-unsafe, and added this comment:

"  Note: This is not fuzzing safe because it can be used to construct\n"
"        deeply nested wrapper chains that cannot exist in the wild."),

Apparently isCallable et al could run out of stack-space.

IMO making functions fuzzing-unsafe is a pretty big (and unfortunate) hammer - we can probably just check for this.
Attached patch PatchSplinter Review
This makes wrapWithProto fuzzing-safe again. It throws an exception when trying to wrap a wrapper to avoid the overrecursion issue. No tests rely on wrapping a wrapper.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8921423 - Flags: review?(jwalden+bmo)
Attachment #8921423 - Flags: review?(jwalden+bmo) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad173cf46012
Make wrapWithProto fuzzing-safe. r=jwalden
https://hg.mozilla.org/mozilla-central/rev/ad173cf46012
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Whiteboard: [js:tech-debt][js:testability] → [js:tech-debt][js:testability][adv-main58-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: