Add a shell builtin to test function relazification

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

({sec-other})

unspecified
mozilla38
sec-other
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [adv-main38-])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 8542965 [details] [diff] [review]
Patch

We've had a number of topcrashes caused by function relazification, the latest one is bug 1115776. These are usually hard to reproduce and track down because they depend on GC timing, JIT compilation of particular scripts etc.

Although it's possible to test relazification in the shell, it requires newGlobal() + eval (see the shell testcase in bug 1115776). The fuzzers do sometimes generate such tests, but I think it's harder or they would have found bug 1115776 a long time ago.

I'm attaching a patch that adds a new shell function, relazify(), to test relazification. This function:

* Must be called directly in the global scope (or it will just throw an exception and do nothing). If this is a problem for the fuzzers (langfuzz always uses eval?), we could maybe come up with something else.

* Accepts exactly the same arguments as gc().

Requesting feedback to see what the fuzzers come up with. It may find problems that won't happen without the patch but hopefully not too many.
Attachment #8542965 - Flags: feedback?(gary)
Attachment #8542965 - Flags: feedback?(choller)
(Assignee)

Comment 1

4 years ago
Created attachment 8542966 [details] [diff] [review]
Patch

Added a simple jit-test that uses this function:

jit-test/tests/basic/relazify.js

In case it helps the fuzzers.
Attachment #8542965 - Attachment is obsolete: true
Attachment #8542965 - Flags: feedback?(gary)
Attachment #8542965 - Flags: feedback?(choller)
Attachment #8542966 - Flags: feedback?(gary)
Attachment #8542966 - Flags: feedback?(choller)
(In reply to Jan de Mooij [:jandem] from comment #0)
>
> * Must be called directly in the global scope (or it will just throw an
> exception and do nothing). If this is a problem for the fuzzers (langfuzz
> always uses eval?), we could maybe come up with something else.

LangFuzz typically uses load() or evaluate(). Would that also work?
(Assignee)

Comment 3

4 years ago
(In reply to Christian Holler (:decoder) from comment #2)
> LangFuzz typically uses load() or evaluate(). Would that also work?

Hm no, there should be only one script frame on the stack, the global script. If you use load or evaluate there will also be a frame for the caller of load/evaluate and it won't work.

Gary, is this also a problem for jsfunfuzz? Does it execute scripts directly or does it load/evaluate them?
(Assignee)

Comment 4

4 years ago
Created attachment 8543271 [details] [diff] [review]
Patch v2

This patch gets rid of the "call in global script" restriction. We now mark the scripts for frames that are on the stack as do-not-relazify, so it no longer matters how relazify() is called.

I temporarily added this behavior to gc() and all jit-tests/jstests passed, so hopefully we can do this...

The fuzzers may find some false positives. If there are too many we can add some restrictions maybe, if there are a few we should just fix them.
Attachment #8542966 - Attachment is obsolete: true
Attachment #8542966 - Flags: feedback?(gary)
Attachment #8542966 - Flags: feedback?(choller)
Attachment #8543271 - Flags: review?(gary)
Attachment #8543271 - Flags: review?(choller)
(In reply to Jan de Mooij [:jandem] from comment #3)
> Gary, is this also a problem for jsfunfuzz? Does it execute scripts directly
> or does it load/evaluate them?

Not directly for jsfunfuzz, but randorderfuzz uses load, so that fuzzer is affected.
Comment on attachment 8543271 [details] [diff] [review]
Patch v2

I gave this a cursory check and it seems to work ok (not blow up anything) on my side. At least, not yet, after half an hour.

(Reviews should go to a reviewer of the code? So setting feedback+)
Attachment #8543271 - Flags: review?(gary) → feedback+
(Assignee)

Comment 7

4 years ago
Comment on attachment 8543271 [details] [diff] [review]
Patch v2

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #6)
> I gave this a cursory check and it seems to work ok (not blow up anything)
> on my side. At least, not yet, after half an hour.

\o/

> (Reviews should go to a reviewer of the code? So setting feedback+)

Oops, when I posted the second patch I used review instead of feedback.
Attachment #8543271 - Flags: review?(choller) → feedback?(choller)
Marking sec-other because it looks like there is no security issue that has been found here.
Keywords: sec-other
(Assignee)

Comment 9

4 years ago
Comment on attachment 8543271 [details] [diff] [review]
Patch v2

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

According to Gary, jsfunfuzz didn't blow up with this patch applied, so we should probably get it in.
Attachment #8543271 - Flags: feedback?(choller) → review?(till)
(In reply to Jan de Mooij [:jandem] from comment #9)
> According to Gary, jsfunfuzz didn't blow up with this patch applied, so we
> should probably get it in.

Hang on, this needs more testing.
Attachment #8543271 - Flags: feedback+ → feedback?(gary)

Comment 11

4 years ago
Is combining relazification with gczeal likely to find more bugs than just testing at user code call points? If so, you should probably make this a setting, like a gcparam, rather than a separate function.

Please add regression tests for everything you can think of, such as:

* Trying to relazify a function that is on the stack
* Trying to relazify a generator that has yielded
* Whatever is going on in bug 1115776 (an interaction with ion?)
(Assignee)

Comment 12

4 years ago
(In reply to Jesse Ruderman from comment #11)
> Is combining relazification with gczeal likely to find more bugs than just
> testing at user code call points? If so, you should probably make this a
> setting, like a gcparam, rather than a separate function.

Separate function is intentional. We don't usually relazify if there are scripts on the stack from that compartment or if the compartment has been entered for another reason. GC's can be triggered in a *lot* more places than we can reenter JS, and we'd get many false positives. 

> Please add regression tests for everything you can think of, such as:
> 
> * Trying to relazify a function that is on the stack
> * Trying to relazify a generator that has yielded
> * Whatever is going on in bug 1115776 (an interaction with ion?)

I wrote a test for bug 1115776 that uses this new relazify() function but we can't add it yet until the fix is on all branches. Can add some others though.
Comment on attachment 8543271 [details] [diff] [review]
Patch v2

Ok, I'm fairly sure this doesn't expose any new bugs, after changing some of our testing code and running overnight.
Attachment #8543271 - Flags: feedback?(gary) → feedback+
Comment on attachment 8543271 [details] [diff] [review]
Patch v2

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

Is there any reason for having this available in non-debug builds? I admit that it might reveal opt-only bugs, but enough so to make it worth having the extra checks and the flag on JSScript?

Anyway, r=me whichever decision you make regarding this.

::: js/src/builtin/TestingFunctions.cpp
@@ +2229,5 @@
>      JS_FN_HELP("gcparam", GCParameter, 2, 0,
>  "gcparam(name [, value])",
>  "  Wrapper for JS_[GS]etGCParameter. The name is one of " GC_PARAMETER_ARGS_LIST),
>  
> +    JS_FN_HELP("relazify", Relazify, 0, 0,

I think I would slightly prefer "relazifyFunctions". (We have other types of laziness in the engine, such as builtins that're installed lazily via resolve hooks.) I don't care enough to hold up the patch over it, though, so up to you.
Attachment #8543271 - Flags: review?(till) → review+
(Assignee)

Comment 15

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb6e90404b76

(In reply to Till Schneidereit [:till] from comment #14)
> Is there any reason for having this available in non-debug builds? I admit
> that it might reveal opt-only bugs, but enough so to make it worth having
> the extra checks and the flag on JSScript?

It's very little extra code so I don't think it matters.

> I think I would slightly prefer "relazifyFunctions". (We have other types of
> laziness in the engine, such as builtins that're installed lazily via
> resolve hooks.)

OK, I renamed it to relazifyFunctions :)
https://hg.mozilla.org/mozilla-central/rev/eb6e90404b76
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Updated

4 years ago
No longer depends on: 1137624
Whiteboard: [adv-main38-]

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.