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)
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)
4.08 KB,
patch
|
h4writer
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•8 years ago
|
||
Regression window is: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ab8a76ac7b34&tochange=1c6385ae1fe7
Reporter | ||
Comment 2•8 years ago
|
||
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
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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 6•8 years ago
|
||
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+
Assignee | ||
Comment 7•8 years ago
|
||
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 8•8 years ago
|
||
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+
Assignee | ||
Comment 9•8 years ago
|
||
thank you again :) will land this after try run, hopefully in 2 hours.
Assignee | ||
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/09aaae5469188c49e28668f407853a25e242aa58 Bug 1267364 - Check isNative every time in GetStringDataProperty. r=h4writer
Reporter | ||
Comment 11•8 years ago
|
||
[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.
status-firefox47:
--- → unaffected
status-firefox48:
--- → affected
status-firefox49:
--- → affected
tracking-firefox48:
--- → ?
Flags: needinfo?(arai.unmht)
Assignee | ||
Comment 12•8 years ago
|
||
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?
Comment 13•8 years ago
|
||
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+
Comment 14•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/09aaae546918
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 15•8 years ago
|
||
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)
Comment 16•8 years ago
|
||
(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)
![]() |
||
Updated•8 years ago
|
Crash Signature: [@ js::Shape::search<T>] → [@ js::Shape::search<T>]
[@ js::Shape* js::Shape::search<T>]
Updated•8 years ago
|
Group: javascript-core-security → core-security-release
Tracking in case this reopens.
Updated•7 years ago
|
Group: core-security-release
Comment 18•7 years ago
|
||
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
status-firefox50:
--- → affected
You need to log in
before you can comment on or make changes to this bug.
Description
•