514563 - ES5 strict mode: arguments.caller and arguments.callee poison pills

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

From ES5 Annex C:

Arguments objects for strict mode functions define non-configurable accessor properties named "caller" and "callee" which throw a TypeError exception on access (10.6).

Comment 1

[Brendan Eich [:brendan]]

Our Error objects (and subtype objects) have a stack property that has string-typed value, which discloses caller and parameter information. ES5 does not forbid this extension in strict mode, but we might want to -- someone worried about string-only, non-object-reference leaks might, anyway.

/be

Comment 2

[Mark S. Miller]

We are indeed worried about such: http://groups.google.com/group/friam/msg/e7c526c1236f0e04?hl=en . I believe this is an example showing the inadequacy of the "Secureable ES5" draft explanation at http://code.google.com/p/es-lab/wiki/SecureableES5 , i.e., it is one of the cases that "What else?" needs to expand to cover.

It would be wonderful if FF3.7's upcoming ES5 implementation were secureable in the sense intended (but not adequately stated) on that page.

A suggestion: have a separate per-frame EphemeronTable off to the side in which Error->stack trace mappings are stored, for those Errors thrown by that frame. Then, these mappings are only accessible to one with access to that EphemeronTable's get() member, which can then be denied by default to SES code.

Comment 3

[Brendan Eich [:brendan]]

EphemeronTable is not necessarily gonna make Fx3.7. Easier to censor .stack in strict mode at runtime.

/be

Comment 4

[Mark S. Miller]

That would certainly solve the "secureable" issue, thanks!

By itself, however, it may impede debugging to the point of deterring use of the "use strict"; directive, which would be a terrible shame. Migration tax and all that... ;).

Or do you mean, censoring the *reading* of .stack by strict code? That would be interesting and surprising, as we have not yet any similar difference between strict and non-strict code. I don't yet know what I would think of that.

Comment 5

[Brendan Eich [:brendan]]

Yes, I meant censoring reading of .stack by strict code.

Strict mode already changes runtime semantics (eval, arguments, the poison pills).

/be

Comment 6

[Mark S. Miller]

What would Object.getOwnPropertyDescriptor(err, 'stack') do when called from either strict or non-strict code?

Comment 7

[Jeff Walden [:Waldo]]

I have a patch done to poison arguments.callee and arguments.caller (by the way, where the heck did the latter come from?  SpiderMonkey doesn't implement it and never has as far as I can tell), just need to write some tests.  More on that tomorrow after some shuteye.

I am going to punt on doing anything to Error.prototype.stack in this bug, because the summary is clear, the desired actions to address it as summarized are clear, and because it seems there's little consensus on what censoring/poisoning the property would actually mean.  It would be helpful if a new bug could be filed for that specific issue, so that progress may occur on the narrow work originally delineated here even while that issue undergoes further discussion.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> I have a patch done to poison arguments.callee and arguments.caller (by the
> way, where the heck did the latter come from?  SpiderMonkey doesn't implement
> it and never has as far as I can tell)

Long ago we did:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=jsfun.c&branch=&root=/cvsroot&subdir=mozilla/js/src&command=DIFF_FRAMESET&rev1=3.11&rev2=3.12

See bug 7224.

/be

Comment 9

[Jeff Walden [:Waldo]]

Interesting.  (I suppose I should note that "as far as I can tell" was imprecise; I was going from memory of never having seen it while hacking on SpiderMonkey, nor having ever seen it while dumpster-diving in CVS history.)

Comment 10

[Jeff Walden [:Waldo]]

Attachment: 1: Rename JSObject::getArgsLength to JSObject::getArgsInitialLength

Since setArgsLength blows away the low bit (bits, soon), I added an assertion to getArgsLength that it was never called on length-overridden arguments.  A skim of getArgsLength calls very quickly demonstrates why that's wrong.  This rename better indicates its semantic meaning.

Comment 11

[Jeff Walden [:Waldo]]

Attachment: 2: Actually poison arguments.callee and arguments.caller

This does the deed, also while adding some named constants (could have been separated into another patch, decided not worth the trouble to disentangle) in places to get rid of some bit-shifting/masking unclarities with more pervasive bit-packing.  It depends on the patch for bug 514581 for the [[ThrowTypeError]] function.

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 462924 [details] [diff] [review]
2: Actually poison arguments.callee and arguments.caller

Hm, this seems to have broken on trace-tests since I last ran it -- retracting until I figure out what I broke.

Comment 13

[Jeff Walden [:Waldo]]

Attachment: 2: Actually poison arguments.callee and arguments.caller

It was a conflict-resolution error while separating out part 1 from part 2 -- didn't fully change the >> in record_JSOP_LENGTH.  This also explains the unnecessary-but-helpful change to arguments/args6.js: the failure I was hitting there was getting a 10 rather than a 5 (for an arguments.length access), and by adding another argument I disambiguated between a bizarre failure that returned the zeroth argument's value and an improperly-shifted actual length.

Comment 11's words continue to apply to this version of the patch.

Comment 14

[David Mandelin [:dmandelin]]

Comment on attachment 462937 [details] [diff] [review]
2: Actually poison arguments.callee and arguments.caller

Something seems wrong just below. There are two ifs in a row with the same condition. Maybe the first one was supposed to be deleted?

>@@ -592,6 +597,41 @@ args_resolve(JSContext *cx, JSObject *ob
>     } else if (JSID_IS_ATOM(id, cx->runtime->atomState.calleeAtom)) {
>         if (!obj->getArgsCallee().isMagic(JS_ARGS_HOLE))
>             valid = true;
>+        if (!obj->getArgsCallee().isMagic(JS_ARGS_HOLE)) {
>+            Value tmp = UndefinedValue();
>+            PropertyOp getter = ArgGetter, setter = ArgSetter;
>+            uintN attrs = JSPROP_SHARED;
>+            if (obj->isArgsStrictMode()) {
>+                PropertyOp throwTypeError = CastAsPropertyOp(GetThrowTypeError(obj));
>+
>+                getter = setter = throwTypeError;
>+                attrs = JSPROP_PERMANENT | JSPROP_GETTER | JSPROP_SETTER | JSPROP_SHARED;
>+            }
>+
>+            if (!js_DefineProperty(cx, obj, id, &tmp, getter, setter, attrs))
>+                return false;
>+            *objp = obj;
>+            return true;
>+        }
>

Otherwise, looks good. I like the new comment:

>diff --git a/js/src/jsobj.h b/js/src/jsobj.h
>--- a/js/src/jsobj.h
>+++ b/js/src/jsobj.h
>@@ -496,9 +496,13 @@ struct JSObject {
>      * Reserved slot structure for Arguments objects:
>      *
>      * JSSLOT_PRIVATE       - the corresponding frame until the frame exits.
>-     * JSSLOT_ARGS_LENGTH   - the number of actual arguments and a flag
>+     * JSSLOT_ARGS_LENGTH   - the number of actual arguments, a flag
>      *                        indicating whether arguments.length was
>-     *                        overwritten.
>+     *                        overwritten, and a flag indicating whether the
>+     *                        function for which arguments were created is in
>+     *                        strict mode.  This slot is not used to represent
>+     *                        arguments.length after that property has been
>+     *                        assigned, even if the new value is integral.
>      * JSSLOT_ARGS_CALLEE   - the arguments.callee value or JSVAL_HOLE if that
>      *                        was overwritten.

Comment 15

[Jeff Walden [:Waldo]]

(In reply to comment #14)
> There are two ifs in a row with the same condition. Maybe the first one was
> supposed to be deleted?

Ah, right (more splitting fallout -- the hard part of narrow patches after-the-fact :-\ ).  With that first one removed (done locally), is all good?  (Aside from needing the dependency first.)

Comment 16

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/faaee481cf7b
http://hg.mozilla.org/tracemonkey/rev/981c0f32ff15

Comment 17

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/981c0f32ff15