Assertion failure: isObject(), at dist/include/js/Value.h:1281

VERIFIED FIXED in Firefox 49

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: gkw, Assigned: arai)

Tracking

(Blocks: 2 bugs, {assertion, sec-moderate, testcase})

Trunk
mozilla49
x86_64
Mac OS X
assertion, sec-moderate, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox47 unaffected, firefox48 wontfix, firefox49 verified, firefox-esr45 unaffected)

Details

(Whiteboard: [jsbugmon:update,ignore][adv-main49+])

Attachments

(6 attachments, 8 obsolete attachments)

42.43 KB, text/plain
Details
13.64 KB, text/plain
Details
4.38 KB, patch
till
: review+
Details | Diff | Splinter Review
3.93 KB, patch
till
: review+
Details | Diff | Splinter Review
3.32 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
6.33 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
The following testcase crashes on mozilla-central revision ab0044bfa1df (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --ion-offthread-compile=off --no-baseline --no-ion):

// Adapted from randomly chosen test: js/src/jit-test/tests/basic/bug1236476.js
oomTest(function() {
    offThreadCompileScript("");
});
// jsfunfuzz-generated
"".match();

Backtrace:

For detailed crash information, see attachment.
(Reporter)

Comment 1

2 years ago
Created attachment 8746024 [details]
Detailed Crash Information
(Reporter)

Comment 2

2 years ago
Created attachment 8746025 [details]
OOM_VERBOSE=1 stack from m-c rev ab0044bfa1df

Not sure if this OOM_VERBOSE=1 stack is relevant.
(Reporter)

Comment 3

2 years ago
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/b5b06959919a
user:        Tooru Fujisawa
date:        Sat Sep 05 21:55:06 2015 +0900
summary:     Bug 887016 - Part 9: Implement RegExp.prototype[@@match] and call it from String.prototype.match. r=till

arai-san, is bug 887016 a likely regressor?
Blocks: 887016
Flags: needinfo?(arai.unmht)
(Reporter)

Comment 4

2 years ago
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   js-dbg-64-dm-clang-darwin-ab0044bfa1df	0x0000000100022c8d JS::Value::toObject() const + 189 (Value.h:1281)
1   js-dbg-64-dm-clang-darwin-ab0044bfa1df	0x000000010004072c js::RegExpPrototypeOptimizable(JSContext*, unsigned int, JS::Value*) + 76 (Value.h:1765)
2   js-dbg-64-dm-clang-darwin-ab0044bfa1df	0x00000001007e3e6e js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 222 (jscntxtinlines.h:236)
3   js-dbg-64-dm-clang-darwin-ab0044bfa1df	0x00000001007d43fe js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 702 (Interpreter.cpp:468)
4   js-dbg-64-dm-clang-darwin-ab0044bfa1df	0x00000001007c9342 Interpret(JSContext*, js::RunState&) + 48322 (Interpreter.cpp:2831)
5   js-dbg-64-dm-clang-darwin-ab0044bfa1df	0x00000001007bd5b7 js::RunScript(JSContext*, js::RunState&) + 519 (Interpreter.cpp:426)
/snip
arai asked to mark it as s-s
Group: core-security
(Assignee)

Comment 6

2 years ago
Thanks :)

This is similar case as `initStringClass`, that happens in bug 1263558.
  https://hg.mozilla.org/integration/mozilla-inbound/rev/b1e8dbf2f4c92666991b0a026dfbc8fa0fa26826

This time it happens in `GlobalObject::resolveConstructor`.

https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/js/src/vm/GlobalObject.cpp#224
>     global->setConstructor(key, ObjectValue(*ctor));
>     global->setConstructorPropertySlot(key, ObjectValue(*ctor));
> 
>     // Define any specified functions and properties, unless we're a dependent
>     // standard class (in which case they live on the prototype), or we're
>     // operating on the self-hosting global, in which case we don't want any
>     // functions and properties on the builtins and their prototypes.
>     if (!StandardClassIsDependent(key) && !cx->runtime()->isSelfHostingGlobal(global)) {
>         if (const JSFunctionSpec* funs = clasp->specPrototypeFunctions()) {
>             if (!JS_DefineFunctions(cx, proto, funs))
>                 return false;
>         }

There, `GlobalObject::setConstructor` is called but the code after that may return with failure on OOM.  In that case, the constructor's `prototype` property is left `undefined`, and `RegExpProto` in the following code becomes `undefined`.

https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/js/src/builtin/String.js#16
> function IsStringMatchOptimizable() {
>     var RegExpProto = GetBuiltinPrototype("RegExp");
>     // If RegExpPrototypeOptimizable succeeds, `exec` and `@@match` are
>     // guaranteed to be data properties.
>     return RegExpPrototypeOptimizable(RegExpProto) &&
>            RegExpProto.exec === RegExp_prototype_Exec &&
>            RegExpProto[std_match] === RegExpMatch;
> }


Then, this time, we cannot move `setConstructor` after defining properties.
In JSProto_Function's case, `Function` constructor is referred from `JS_DefineFunctions`, so we should call `setConstructor` *before* it to make it accessible.

Thus, we should reset constructor slot when any of operations *after* `setConstructor` fails.
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 7

2 years ago
Created attachment 8746050 [details] [diff] [review]
Part 1: Reset constructor slot of GlobalObject to undefined when it fails to initialize constructor.

Changed all "return false" after `setConstructor` to "goto failedAfterSetConstructor", and in `failedAfterSetConstructor:` label, there `setConstructor` and `setConstructorPropertySlot` are called with `UndefinedValue()`, so that `resolveConstructor` will be called again on next time.
Assignee: nobody → arai.unmht
Attachment #8746050 - Flags: review?(till)
(Assignee)

Comment 8

2 years ago
Created attachment 8746056 [details] [diff] [review]
Part 2: Call setConstructor and initBuiltinConstructor after defining properties in all init function.

Also, there are some more cases that setConstructor and initBuiltinConstructor are called before defining properties.
Attachment #8746056 - Flags: review?(till)
Comment on attachment 8746050 [details] [diff] [review]
Part 1: Reset constructor slot of GlobalObject to undefined when it fails to initialize constructor.

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

Hmm, do we need to use goto here? Can't we not move all the code that contains the gotos into a new function and do "if (!foo(..)) {do the reset stuff; return false;} else {return true;}?
Attachment #8746050 - Flags: review?(till)
(Assignee)

Comment 10

2 years ago
Created attachment 8746079 [details] [diff] [review]
Part 1: Reset constructor slot of GlobalObject to undefined when it fails to initialize constructor.

Thank you for prompt response!

Changed it to a function.  Do you know any nice shorter name for the function?
Attachment #8746050 - Attachment is obsolete: true
Attachment #8746079 - Flags: review?(till)
Comment on attachment 8746079 [details] [diff] [review]
Part 1: Reset constructor slot of GlobalObject to undefined when it fails to initialize constructor.

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

Thanks, that looks much better. The name is not fantastic, but I can't think of anything better there, so let's keep it.
Attachment #8746079 - Flags: review?(till) → review+
Comment on attachment 8746056 [details] [diff] [review]
Part 2: Call setConstructor and initBuiltinConstructor after defining properties in all init function.

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

r=me
Attachment #8746056 - Flags: review?(till) → review+
(Assignee)

Comment 13

2 years ago
Comment on attachment 8746079 [details] [diff] [review]
Part 1: Reset constructor slot of GlobalObject to undefined when it fails to initialize constructor.

Thank you for reviewing :D

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
This particular case (the testcase in comment #0) is not so easy, as it's just a null dereference, and it also needs testing functions that are not exposed to web content.
For underlying issue, I think everything else would also be a null dereference too, but I'm not sure how widely the issue affects.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The testcase describes how to cause crash, but with testing functions.

> Which older supported branches are affected by this flaw?
This particular flaw affects mozilla49 (nightly) and mozilla48 (aurora).
The underlying issue affects all branches.

> If not all supported branches, which bug introduced the flaw?
bug 887016 (mozilla48)

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Not yet, but it should be easily created.
Also, whole patches related to bug 887016 will be backed out from aurora in bug 1265307 anyway.

> How likely is this patch to cause regressions; how much testing does it need?
Less likely.  Automated tests should be able to catch anything happens.
Attachment #8746079 - Flags: sec-approval?
If this is just a null dereference, why are you asking for sec-approval?

This bug needs a security rating but null dereferences are not normally sec-high or sec-critical, which are the only rating that require security approvals. I assume that this is a sec-low if it really is just a null dereference.

https://wiki.mozilla.org/Security/Bug_Approval_Process
status-firefox47: --- → unaffected
status-firefox48: --- → affected
status-firefox-esr45: --- → unaffected
Flags: needinfo?(dveditz)
Flags: needinfo?(dveditz)
Keywords: sec-moderate
(Assignee)

Comment 15

2 years ago
Comment on attachment 8746079 [details] [diff] [review]
Part 1: Reset constructor slot of GlobalObject to undefined when it fails to initialize constructor.

Thanks!
clearing sec-approval.  will land them shortly.
Attachment #8746079 - Flags: sec-approval?
(Assignee)

Comment 16

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d31171af48e4fe4e0e35e6e96b4c76504a7ee091
Bug 1268034 - Part 1: Reset constructor slot of GlobalObject to undefined when it fails to initialize constructor. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/05c1151be7a7821136263bb46d887bf8e99ce64d
Bug 1268034 - Part 2: Call setConstructor and initBuiltinConstructor after defining properties in all init function. r=till
https://hg.mozilla.org/mozilla-central/rev/d31171af48e4
https://hg.mozilla.org/mozilla-central/rev/05c1151be7a7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

2 years ago
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified

Comment 18

2 years ago
JSBugMon: This bug has been automatically verified fixed.
(Assignee)

Comment 19

2 years ago
Comment on attachment 8746079 [details] [diff] [review]
Part 1: Reset constructor slot of GlobalObject to undefined when it fails to initialize constructor.

same patches are applicable to mozilla-aurora

Approval Request Comment
> [Feature/regressing bug #]
bug 887016 exposed the issue, but underlying issue exists from old branches.

> [User impact if declined]
Unnecessary crash when hitting potentially-recoverable OOM, by opening certain web page.

> [Describe test coverage new/current, TreeHerder]
Tested in mozilla-central automated test.

> [Risks and why]
Low.  Added recover code path for OOM.

> [String/UUID change made/needed]
None
Attachment #8746079 - Flags: approval-mozilla-aurora?
(Reporter)

Updated

2 years ago
Depends on: 1269074
(Assignee)

Comment 20

2 years ago
Comment on attachment 8746079 [details] [diff] [review]
Part 1: Reset constructor slot of GlobalObject to undefined when it fails to initialize constructor.

withdrawing approval, due to bug 1269074.
Attachment #8746079 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 21

2 years ago
So, we should do some more steps to rollback *all* operations done in GlobalObject::resolveConstructor when it fails.
(Assignee)

Comment 22

2 years ago
at least, the following step should be rolled back, or skipped on the 2nd time

>    if (clasp->specShouldDefineConstructor()) {
>        if (!global->addDataProperty(cx, id, constructorPropertySlot(key), 0))
>            return false;
>    }

Also, the following should also be rolled back, as the prototype properties are not yet initialized.

>        global->setPrototype(key, ObjectValue(*proto));
(Assignee)

Comment 23

2 years ago
and now I'm wondering if it's really recoverable.
Could it be possible that each hook (specCreateConstructorHook, specFinishInitHook) do un-recoverable operation?
(Assignee)

Comment 24

2 years ago
for example, FinishObjectClassInit defines property on global object.
  https://dxr.mozilla.org/mozilla-central/rev/8c3fd523d75bd30f691ca2d6cfdad18d576392a1/js/src/builtin/Object.cpp#1194

this specific case could be ignored tho, I'm not sure if other (and future) cases are also recoverable.

should we reduce the target of recover operation?
or perhaps, should we throw away the assumption that constructor/prototype are initialized properly, especially even if the property is non-configurable? (I hope not...)


till, can I have your opinion for comment #21-#23, and bug 1269074's case?
Flags: needinfo?(till)
(Assignee)

Updated

2 years ago
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Updated

2 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]

Comment 25

2 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 2b7c421063ad).
(Assignee)

Comment 26

2 years ago
So, the issue initially hit with this not-properly-initialized constructor object was bug 1263558, there String.prototype was undefined.
and this bug is that RegExp.prototype was undefined.

we didn't check them because of the following assumption:

  "prototype" property should always be a prototype object for each constructor, because "prototype" property is non-configurable data property

But if constructor/prototype initialization can fail with OOM and can leave something partially-initialized, those assumption breaks.  This is also true for all other non-configurable properties if any (will check the spec).

Now we're going to rollback the initialization to make those partially-initialized object not-accessible from other code, and then perform the initialization again on the next time when the constructor/prototype is accessed.

So, those initialization operation should be able to be performed more than once, until the initialization completes without any error.

I'm not sure if that rule can be maintained...
(Assignee)

Comment 27

2 years ago
In addition to "prototype" property, the following properties are non-configurable.

  Function.prototype[@@hasInstance]
  Symbol.hasInstance
  Symbol.isConcatSpreadable
  Symbol.iterator
  Symbol.match
  Symbol.prototype
  Symbol.replace
  Symbol.search
  Symbol.species
  Symbol.split
  Symbol.toPrimitive
  Symbol.toStringTag
  Symbol.unscopables
  Number.EPSILON
  Number.MAX_SAFE_INTEGER
  Number.MAX_VALUE
  Number.MIN_SAFE_INTEGER
  Number.MIN_VALUE
  Number.NaN
  Number.NEGATIVE_INFINITY
  Number.POSITIVE_INFINITY
  Math.E
  Math.LN10
  Math.LN2
  Math.LOG10E
  Math.LOG2E
  Math.PI
  Math.SQRT1_2
  Math.SQRT2
  %TypedArray%.BYTES_PER_ELEMENT
  %TypedArray%.prototype.BYTES_PER_ELEMENT

and Function and TypedArray uses InitViaClassSpec.
(Assignee)

Comment 28

2 years ago
Created attachment 8747412 [details] [diff] [review]
(WIP) Part 3: Delete global property when it fails to initialize constructor.

In case we should go with current approach, here's the patch that removes the constructor property from global object, so that the property is not in the global object's shape on the next time.

NativeDeleteProperty is fallible, and in failure case we cannot recover anymore, added MOZ_CRASH there but I'm not sure what's the best way there...
Attachment #8747412 - Flags: feedback?(till)
(Assignee)

Comment 29

2 years ago
Created attachment 8747413 [details] [diff] [review]
Part 4: Reset prototype slot of GlobalObject to undefined when it fails to initialize prototype.

and one more patch to reset prototype slot of global object on failure case.
Attachment #8747413 - Flags: review?(till)
Comment on attachment 8747412 [details] [diff] [review]
(WIP) Part 3: Delete global property when it fails to initialize constructor.

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

r=me
Attachment #8747412 - Flags: feedback?(till) → feedback+
Comment on attachment 8747413 [details] [diff] [review]
Part 4: Reset prototype slot of GlobalObject to undefined when it fails to initialize prototype.

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

r=me
Attachment #8747413 - Flags: review?(till) → review+
(Assignee)

Comment 32

2 years ago
Thank you for reviewing :)

Now checking each hook.
so far, CreateFunctionPrototype cannot be executed twice...

https://dxr.mozilla.org/mozilla-central/rev/1461a4071341c282afcf7b72e33036412d2251d4/js/src/jsfun.cpp#832
> static JSObject*
> CreateFunctionPrototype(JSContext* cx, JSProtoKey key)
> {
>     Rooted<GlobalObject*> self(cx, cx->global());
> ...
>     self->setThrowTypeError(throwTypeError);

https://dxr.mozilla.org/mozilla-central/rev/1461a4071341c282afcf7b72e33036412d2251d4/js/src/vm/GlobalObject.h#151
>     void setThrowTypeError(JSFunction* fun) {
>         MOZ_ASSERT(getSlotRef(THROWTYPEERROR).isUndefined());
>         setSlot(THROWTYPEERROR, ObjectValue(*fun));
>     }

The assertion fails.


This could happen in the following case:
  1. resolve Function constructor
  2. call createPrototype hook (CreateFunctionPrototype)
  3. OOM happens while defining prototype property
  4. tries to rollback, reset constructor slot (Part 1)

  5. resolve Function constructor because constructor slot is undefined
  6. call createPrototype hook (CreateFunctionPrototype) again


So we may need extra function to rollback each hook, or detect the 2nd execution inside each hook.
(Assignee)

Comment 33

2 years ago
Also TypedArray's finishClassInit cannot be executed twice.

>     static bool
>     finishClassInit(JSContext* cx, HandleObject ctor, HandleObject proto)
>     {
> ....
>         cx->global()->setCreateArrayFromBuffer<NativeType>(fun);
>         return true;
>     }

>     void setCreateArrayFromBufferHelper(uint32_t slot, Handle<JSFunction*> fun) {
>         MOZ_ASSERT(getSlotRef(slot).isUndefined());
>         setSlot(slot, ObjectValue(*fun));
>     }

the assertion fails.


Object's FinishObjectClassInit seems to be also problematic.

> static bool
> FinishObjectClassInit(JSContext* cx, JS::HandleObject ctor, JS::HandleObject proto)
> {
> ...
>     global->setOriginalEval(evalobj);
> ...
>     if (global->shouldSplicePrototype(cx)) {
>         if (!global->splicePrototype(cx, global->getClass(), tagged))
>             return false;
>     }


>     void setOriginalEval(JSObject* evalobj) {
>         MOZ_ASSERT(getSlotRef(EVAL).isUndefined());
>         setSlot(EVAL, ObjectValue(*evalobj));
>     }

the assertion in setOriginalEval fails.


> bool
> JSObject::shouldSplicePrototype(JSContext* cx)
> {
>     /*
>      * During bootstrapping, if inference is enabled we need to make sure not
>      * to splice a new prototype in for Function.prototype or the global
>      * object if their __proto__ had previously been set to null, as this
>      * will change the prototype for all other objects with the same type.
>      */
>     if (getProto() != nullptr)
>         return false;
>     return isSingleton();
> }

I suppose `getProto() != nullptr` is different on 1st and 2nd execution.
(Assignee)

Comment 34

2 years ago
now I'm thinking another way.

Can't we invalidate the global once constructor/prototype initialization fails with OOM?
like, setting yet another value to the constructor/prototype slot (maybe, a magic value?), and detect it and throw InternalError every time the script (or the internal) tries to access it.

So that we won't use the partially-initialized constructor/prototype anywhere, and we won't execute resolveConstructor twice.
of course the global cannot execute any of script from there, but it happens only when it hit OOM.
I feel it's safer than trying to recover from OOM.
(of course we have to make sure that it won't fall into infinite loop of throwing error tho)
Bah, very annoying. I'm not sure the proposed solution really works. Perhaps at this point we should prevent all script execution in the affected global? I think we have the infrastructure for that.

Needinfo'ing jorendorff because I can't think of a good solution here that isn't somewhat radical.
Flags: needinfo?(till) → needinfo?(jorendorff)
(Assignee)

Comment 36

2 years ago
Created attachment 8747501 [details] [diff] [review]
(plan B, WIP) Part 3: Add GlobalObject::overrideConstructor and use it instead of GlobalObject::setConstructor when overriding.

WIP patches for the comment #34 idea, based on backing out Part 1.

First, Part 3 adds GlobalObject::overrideConstructor to distinguish between "newly setting constructor" and "overriding existing constructor".
(Assignee)

Comment 37

2 years ago
Created attachment 8747502 [details] [diff] [review]
(plan B, WIP) Part 4: Mark constructor/prototype slot when initialization failed with OOM.

Then, Part 4 adds new magic value to mark constructor/prototype slot that initialization is failed before.
(Assignee)

Comment 38

2 years ago
Created attachment 8747525 [details] [diff] [review]
(plan B, WIP) Part 4: Mark constructor/prototype slot when initialization failed with OOM.

forgot to refresh the patch
Attachment #8747502 - Attachment is obsolete: true
Group: core-security → javascript-core-security
The list of non-configurable properties does not seem like a problem to me. Math.PI, for example - what exactly can go wrong there?

The problem with the GlobalObject's prototypes and constructors is serious. If possible, we should always create the prototype-constructor pair and fully populate it before storing those objects in the GlobalObject. The property should be defined, and the reserved slots should be populated, only after everything else has succeeded. That way there is nothing to roll back on failure.

Arai, if I fix the %%ObjectPrototype%% and %%FunctionPrototype%% initialization, can the rest be fixed as described above?
Flags: needinfo?(jorendorff) → needinfo?(arai.unmht)
(Assignee)

Comment 40

2 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #39)
> The list of non-configurable properties does not seem like a problem to me.
> Math.PI, for example - what exactly can go wrong there?

just wanted to list up possibly affected properties.  if they're not problem, we just have to worry about 'prototype' property :)

Indeed, most of them won't be a problem, as those are really constants, and self-hosted code doesn't use those properties but global variables or macro like std_iterator or MAX_*.


> The problem with the GlobalObject's prototypes and constructors is serious.
> If possible, we should always create the prototype-constructor pair and
> fully populate it before storing those objects in the GlobalObject. The
> property should be defined, and the reserved slots should be populated, only
> after everything else has succeeded. That way there is nothing to roll back
> on failure.

Yeah, if it's possible, it should be much better solution than rollback or invalidate.


> Arai, if I fix the %%ObjectPrototype%% and %%FunctionPrototype%%
> initialization, can the rest be fixed as described above?

Will investigate it shortly.
Thanks!
(Assignee)

Comment 41

2 years ago
For other cases than Function/Object, the issue is that the number of fallible operations that modifies global object, and we may have to perform small-rollback on failure case, or skip them on 2nd exection.


Here's the list of operations that modifies global object, done in resolveConstructor:
  fallible
    global->addDataProperty(cx, id, constructorPropertySlot(key), 0)
    finishInit(cx, ctor, proto)

  infallible
    global->setConstructor(key, ObjectValue(*ctor));
    global->setConstructorPropertySlot(key, ObjectValue(*ctor));
    global->setPrototype(key, ObjectValue(*proto));
    AddTypePropertyId(cx, global, id, ObjectValue(*ctor));

finishInit may modify global object (TypedArray's and Object's case, see comment #32), and there could be more than 1 operations inside.

for TypedArray's case, setCreateArrayFromBuffer itself is infallible, but other operations there (DefineProperty, NewNativeFunction) are fallible.  Anyway, as long as there are fallible operations inside finishInit, there is no good ordering between finishInit and addDataProperty, that can avoid rollback or skip.

So, we should do either:
  a) rollback addDataProperty when finishInit fails
  b) do not perform addDataProperty on 2nd execution (but assert that it has exactly same property)


Also, if finishInit itself may contain more than 1 fallible operations that modifies global object, finishInit should handle the failure case.
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 42

2 years ago
Created attachment 8748505 [details] [diff] [review]
WIP: Delay modifying global constructor/prototype slots for classes other than Function and Object.

Here's WIP patch that modifies global slots/properties after populating/linking constructor/prototypes, for non-Function/Object.
It passes jstests and jittest, so I think everything else than Function/Object can be fixed with this way.
Attachment #8747412 - Attachment is obsolete: true
Attachment #8747413 - Attachment is obsolete: true
Attachment #8747501 - Attachment is obsolete: true
Attachment #8747525 - Attachment is obsolete: true
I think the part of typed arrays' finishClassInit hook that modifies the global object should be removed.

Instead, at the point in fromBufferWithProto where it calls cx->global()->createArrayFromBuffer<NativeType>, it should create that function on demand, and cache it on success.
finishClassInit might be a bad design to begin with. The way to do a complex fallible operation is to do as much work as possible in isolation, then make it public ("commit" it) in a single step. (cf. transactions)

So having a fallible hook that's called *after* class initialization is committed (by defining a global property) is questionable.
(Assignee)

Comment 45

2 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #43)
> I think the part of typed arrays' finishClassInit hook that modifies the
> global object should be removed.
> 
> Instead, at the point in fromBufferWithProto where it calls
> cx->global()->createArrayFromBuffer<NativeType>, it should create that
> function on demand, and cache it on success.

Okay, will fix it first.


(In reply to Jason Orendorff [:jorendorff] from comment #44)
> finishClassInit might be a bad design to begin with. The way to do a complex
> fallible operation is to do as much work as possible in isolation, then make
> it public ("commit" it) in a single step. (cf. transactions)
> 
> So having a fallible hook that's called *after* class initialization is
> committed (by defining a global property) is questionable.

There are 2 kind of things done in finishInit hook,

  A) modifies constructor or prototype object
  B) modifies global object

and (A) won't be a problem, and it will make things easier,
also it can be done as a part of constructor/prototype initialization.

So, how about reducing the things can be done in finishInit hook only to (A) ?


Here's the list of things done in each prototype's finishInit hook.

  Object -- FinishObjectClassInit
    (B) defines global.eval
    (B) stores the original eval function to global (global->setOriginalEval)
    (B) install the intrinsics holder to global (GlobalObject::getIntrinsicsHolder)
    (B) splice global object's prototype (global->splicePrototype)

  Array -- array_proto_finish
    (A) defines Array.prototype[@@unscopables]

  Date -- FinishDateClassInit
    (A) defines Date.prototype.toUTCString
    (A) defines Date.prototype.toGMTString

  TypedArray -- _type##Array::finishClassInit
    (A) defines ctor.BYTES_PER_ELEMENT
    (A) defines ctor.prototype.BYTES_PER_ELEMENT
    (B) creates `CreateArrayFromBuffer` function and stores it to global (cx->global()->setCreateArrayFromBuffer)
        => move to other place

  SavedFrame -- SavedFrame::finishSavedFrameInit
    (A) stores NullValue to SavedFrame.prototype's SavedFrame::JSSLOT_SOURCE slot
    (A) freezes SavedFrame.prototype


Once TypedArray's case is fixed, Object's case is only the remaining issue, and I think it should be done in different place than finishInit hook, as most of them are not related to Object prototype itself, but just a dependencies.
(Assignee)

Comment 46

2 years ago
Created attachment 8749363 [details] [diff] [review]
Part 3: Create CreateArrayFromBuffer function on demand.

Moved ArrayBufferObject::createTypedArrayFromBuffer function creation to dedicated function, that is called when invoking it.

`!getSlot(slot).isUndefined()` assertion in GlobalObjectGlobalObject:createArrayFromBufferHelper is removed as we need to access it to check if it's cached or not.
Attachment #8749363 - Flags: review?(jorendorff)
(Assignee)

Comment 47

2 years ago
Created attachment 8749365 [details] [diff] [review]
(WIP) Part 4: Delay modifying global constructor/prototype slots for classes other than Function and Object.

now finishInit hook doesn't modify global for non-Object case.
moved it to initialization.
Attachment #8748505 - Attachment is obsolete: true
Thank you for the list in comment 45. That's great work.
Comment on attachment 8749363 [details] [diff] [review]
Part 3: Create CreateArrayFromBuffer function on demand.

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

r=me. Thanks!
Attachment #8749363 - Flags: review?(jorendorff) → review+
(Reporter)

Comment 50

2 years ago
Arai-san, what's next here?
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 51

2 years ago
Thank you for reminder :)

So, the already-confirmed issues (RegExp and String prototypes) can be fixed by, backing out Part 1, and landing Part 3 and Part 4 (it needs cleanup tho),
but it leaves the issue on Object and Function prototypes.

jorendorff, what's your plan about Object and Function prototypes, regarding comment #39 ?
can these be left for now?
Flags: needinfo?(arai.unmht) → needinfo?(jorendorff)
If you can land a fix for everything else, land it.

Fixing Object and Function startup is a lot more work. I have a partial patch, but no time to work on it.
Flags: needinfo?(jorendorff)
(Assignee)

Comment 53

2 years ago
Created attachment 8753911 [details] [diff] [review]
Part 4: Delay modifying global constructor/prototype slots for classes other than Function and Object.

Thanks :)

Reduced duplicated code.

This patch changes the order of initialization for non-Object/Function, and do operations that modifies global object after all other things.
Attachment #8749365 - Attachment is obsolete: true
Attachment #8753911 - Flags: review?(jorendorff)
Comment on attachment 8753911 [details] [diff] [review]
Part 4: Delay modifying global constructor/prototype slots for classes other than Function and Object.

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

Great. Thanks.

::: js/src/vm/GlobalObject.cpp
@@ +266,5 @@
> +    if (!isObjectOrFunction) {
> +        // Any operations that modifies global object should be placed after
> +        // any other fallible operations.
> +
> +        // Fallible operations that modifies global object.

Grammar nit:
        // Fallible operation that modifies the global object.

@@ +272,5 @@
> +            if (!global->addDataProperty(cx, id, constructorPropertySlot(key), 0))
> +                return false;
> +        }
> +
> +        // Infallible operations that modifies global object.

// Infallible operations that modify the global object.
Attachment #8753911 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 55

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7861d6b6c274db85036adc5496718d456e3a78c8
Backed out changeset d31171af48e4 (bug 1268034)

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c2a58d84d498bd8841f3bd88e6091ebcba58818
Bug 1268034 - Part 3: Create CreateArrayFromBuffer function on demand. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e198b4ccab26156c3dd6417b2ee08d53ab622dd
Bug 1268034 - Part 4: Delay modifying global constructor/prototype slots for classes other than Function and Object. r=jorendorff
https://hg.mozilla.org/mozilla-central/rev/7861d6b6c274
https://hg.mozilla.org/mozilla-central/rev/9c2a58d84d49
https://hg.mozilla.org/mozilla-central/rev/1e198b4ccab2
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Comment 57

2 years ago
The testcase in comment #0 is specific to bug 887016.

The underlying issue fixed by patches in this bug affects all branches tho, that won't be critical for other branches than m-c.
Group: javascript-core-security → core-security-release

Updated

2 years ago
Status: RESOLVED → VERIFIED

Comment 58

2 years ago
JSBugMon: This bug has been automatically verified fixed.
status-firefox48: affected → wontfix
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][adv-main49+]

Updated

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