Closed Bug 1267364 Opened 8 years ago Closed 8 years ago

topcrash in js::Shape::search<T> starting in 2016-04-25 builds, EXCEPTION_ACCESS_VIOLATION_READ crashes at 0xd or 0x15

Categories

(Core :: JavaScript Engine, defect)

x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- unaffected
firefox48 + fixed
firefox49 --- fixed
firefox50 --- affected

People

(Reporter: dbaron, Assigned: arai)

References

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(1 file, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-4692e210-2348-488e-92ab-e76452160425.
=============================================================

There's a new topcrash in today's builds, at very large numbers (the point where we might want to respin nightly with a backout).  In most cases, the URL is  https://e.mail.ru/messages/inbox/ .

The crash address is always 0xd on x86 and 0x15 on amd64.

(There were previously a small number of crashes with this crash signature, but they weren't EXCEPTION_ACCESS_VIOLATION_READ crashes with those crash addresses.)
Flags: needinfo?(nihsanullah)
Frame #3 of the stack goes through code added in https://hg.mozilla.org/mozilla-central/rev/8b08faebf408 which is in that range
Blocks: 1264264
Flags: needinfo?(nihsanullah) → needinfo?(arai.unmht)
Arai-san requests for this to be s-s over IRC.
Group: javascript-core-security
GetElemBaseForLambda checked elemBase is native, but it can be swapped before GetStringDataProperty, by JSObject::swap, so added isNative check to GetStringDataProperty.
Assignee: nobody → arai.unmht
Flags: needinfo?(arai.unmht)
Attachment #8745047 - Flags: review?(hv1989)
Sorry, forgot to store the fix for the comment in GetStringDataProperty

> +        // The object is already checked to be native in GetElemBaseForLambda,
> +        // but it can be swapped to the other class that is non-native.
Comment on attachment 8745047 [details] [diff] [review]
Check isNative every time in GetStringDataProperty.

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

Can you also assert on:
https://hg.mozilla.org/mozilla-central/rev/8b08faebf408#l4.79

That the return value of GetStringDataProperty is an string?

::: js/src/builtin/RegExp.cpp
@@ +1712,5 @@
>      MOZ_ASSERT(args.length() == 2);
>  
> +    RootedObject obj(cx, &args[0].toObject());
> +    if (!obj->isNative()) {
> +        // The object is already chekced to be native, but it can be swapped.

Next to the changes you mentioned in the above comment. Please also add:
// Return undefined to mark failure to get the property.

::: js/src/jsarray.cpp
@@ +3211,5 @@
>  bool
>  js::ArrayConstructor(JSContext* cx, unsigned argc, Value* vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    return ArrayConstructorImpl(cx, args, /* isConstructor = */ true);

Not part of this patch.

@@ +3221,5 @@
>      CallArgs args = CallArgsFromVp(argc, vp);
>      MOZ_ASSERT(!args.isConstructing());
>      MOZ_ASSERT(args.length() == 1);
>      MOZ_ASSERT(args[0].isNumber());
> +    return ArrayConstructorImpl(cx, args, /* isConstructor = */ false);

Ditto.
Attachment #8745047 - Flags: review?(hv1989) → review+
Based on the discussion on IRC, changed the testcase to use really non-native object: Proxy, unboxed object, and typed object
Attachment #8745047 - Attachment is obsolete: true
Attachment #8745088 - Flags: review?(hv1989)
Comment on attachment 8745088 [details] [diff] [review]
Check isNative every time in GetStringDataProperty. r=h4writer

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

Awesome testcase!
Attachment #8745088 - Flags: review?(hv1989) → review+
thank you again :)

will land this after try run, hopefully in 2 hours.
https://hg.mozilla.org/integration/mozilla-inbound/rev/09aaae5469188c49e28668f407853a25e242aa58
Bug 1267364 - Check isNative every time in GetStringDataProperty. r=h4writer
[Tracking Requested - why for this release]:

This needs to land on Aurora as well, since there was an uplift today.  (Or the other patch should be backed out on Aurora.)

And it should land *before* Aurora updates are turned on.  It is by far the #1 topcrash on nightly.
Flags: needinfo?(arai.unmht)
Comment on attachment 8745088 [details] [diff] [review]
Check isNative every time in GetStringDataProperty. r=h4writer

asking approval in advance in order to land this quickly.

Approval Request Comment
[Feature/regressing bug #]: bug 1264264
[User impact if declined]: topcrash
[Describe test coverage new/current, TreeHerder]: tested in mozilla-inbound
[Risks and why]: Low, this adds one more fallback path for the unhandled case, the fallback path was already tested.
[String/UUID change made/needed]: None
Flags: needinfo?(arai.unmht)
Attachment #8745088 - Flags: approval-mozilla-aurora?
No longer depends on: 1267565
Comment on attachment 8745088 [details] [diff] [review]
Check isNative every time in GetStringDataProperty. r=h4writer

Taking it before we enable updates.
Attachment #8745088 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/09aaae546918
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
has problems applying to aurora:

merging js/src/builtin/RegExp.cpp                                                                                                                                                                           
warning: conflicts while merging js/src/builtin/RegExp.cpp! (edit, then use 'hg resolve --mark')                                                                                                            
abort: unresolved conflicts, can't continue                                                                                                                                                                 
(use hg resolve and hg graft --continue)
Tomcats-MacBook-Pro-2:mozilla-central Tomcat$
Flags: needinfo?(arai.unmht)
(In reply to Carsten Book [:Tomcat] from comment #15)
> has problems applying to aurora:
> 

oh wait, my bad.

Pushed now https://hg.mozilla.org/releases/mozilla-aurora/rev/cbeae0c4410b
Flags: needinfo?(arai.unmht)
Crash Signature: [@ js::Shape::search<T>] → [@ js::Shape::search<T>] [@ js::Shape* js::Shape::search<T>]
Group: javascript-core-security → core-security-release
Tracking in case this reopens.
Group: core-security-release
Crash volume for signature 'js::Shape::search<T>':
 - nightly (version 50): 8 crashes from 2016-06-06.
 - aurora  (version 49): 30 crashes from 2016-06-07.
 - beta    (version 48): 108 crashes from 2016-06-06.
 - release (version 47): 650 crashes from 2016-05-31.
 - esr     (version 45): 0 crash from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          2          1          0          0          2          2          1
 - aurora           7          0          3          4          3         10          1
 - beta            11         18          6         30         20         15          5
 - release        192        116        120         78         37         32          6
 - esr              0          0          0          0          0          0          0

Affected platforms: Windows, Linux
You need to log in before you can comment on or make changes to this bug.