Closed
Bug 784342
Opened 12 years ago
Closed 12 years ago
Crash [@ js::HeapSlot::init]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
DUPLICATE
of bug 784345
People
(Reporter: decoder, Unassigned)
References
Details
(Keywords: crash, testcase, Whiteboard: [jsbugmon:update])
Crash Data
Attachments
(1 file)
2.63 KB,
patch
|
Details | Diff | Splinter Review |
The following test crashes on mozilla-central revision f9a8fdb08193 (options -m -n):
var a = new Array(1000 * 1000);
new ParallelArray(a);
Valgrind shows:
==39020== Invalid read of size 8
==39020== at 0x46AD57: js::HeapSlot::init(JSCompartment*, JSObject*, unsigned int, JS::Value const&) (Barrier-inl.h:264)
==39020== by 0x46B8EE: JSObject::initDenseArrayElements(unsigned int, JS::Value const*, unsigned int) (jsobjinlines.h:479)
==39020== by 0x47ADDB: js::NewDenseCopiedArray(JSContext*, unsigned int, JS::Value const*, JSObject*) (jsarray.cpp:3789)
==39020== by 0x715CBE: NewDenseArrayWithType(JSContext*, unsigned int, JS::Handle<JSObject*>) (ParallelArray.cpp:47)
==39020== by 0x719AB5: js::ParallelArrayObject::construct(JSContext*, unsigned int, JS::Value*) (ParallelArray.cpp:1020)
==39020== by 0x52E22D: js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:389)
==39020== by 0x52E371: js::CallJSNativeConstructor(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:424)
==39020== by 0x5370FE: js::InvokeConstructorKernel(JSContext*, JS::CallArgs) (jsinterp.cpp:417)
==39020== by 0x544F1B: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2420)
==39020== by 0x53688F: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:308)
==39020== by 0x53764D: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) (jsinterp.cpp:492)
==39020== by 0x537898: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (jsinterp.cpp:530)
==39020== Address 0x7200000 is not stack'd, malloc'd or (recently) free'd
This is after most fixes for previous ParallelArray issues landed so I assume this is a separate problem. S-s due to possibly dangerous crash.
Reporter | ||
Updated•12 years ago
|
Crash Signature: [@ js::HeapSlot::init]
[@ js::NewDenseCopiedArray] → [@ js::HeapSlot::init]
[@ js::NewDenseCopiedArray]
Comment 2•12 years ago
|
||
Attachment #653888 -
Flags: review?(dmandelin)
Comment 3•12 years ago
|
||
On second thought, it is unclear to me if it makes sense to initialize holes in the PA backing store -- it is also possible to eagerly initialize all the holes into undefineds, which then avoids the issue of being able to get holes out of ParallelArray objects in bug 784345.
Crash Signature: [@ js::HeapSlot::init]
[@ js::NewDenseCopiedArray] → [@ js::HeapSlot::init]
[@ js::NewDenseCopiedArray]
Comment 4•12 years ago
|
||
Comment on attachment 653888 [details] [diff] [review]
fix and testcase
Review of attachment 653888 [details] [diff] [review]:
-----------------------------------------------------------------
Hmmm. To me, it seems like it would be better to just change NewDenseCopiedArray so that it just works in this situation. But I'm not entirely sure that's the right thing to do--maybe we're relying on the assertion inside for testing purposes. I'd ask Jeff but he's on vacation. What do you think, Shu?
::: js/src/builtin/ParallelArray.cpp
@@ +40,5 @@
> }
>
> +// Initialize a new dense array and copy everything we can from the dense
> +// array source, truncating to the new length. The difference in lengths is
> +// fliled with holes.
typo: s/fliled/filled
Comment 5•12 years ago
|
||
(In reply to Shu-yu Guo [:shu] from comment #3)
> On second thought, it is unclear to me if it makes sense to initialize holes
> in the PA backing store -- it is also possible to eagerly initialize all the
> holes into undefineds, which then avoids the issue of being able to get
> holes out of ParallelArray objects in bug 784345.
That is a very good question. I'm not sure but I think that might work. AFAIK the only use of holes is so that if the array has no property named |2|, and the prototype does, you can see through to the prototype's |2|. Which is of course awful, but what we're presently stuck with for regular arrays.
Luke pointed out that ParallelArray might be able to change things. If ParallelArray objects are dense by definition, then you don't need the holes. I don't think the spec is complete enough to say for sure, but what's in there is generally consistent with that notion, so I'd say go for it.
Comment 6•12 years ago
|
||
(In reply to David Mandelin [:dmandelin] from comment #5)
> Luke pointed out that ParallelArray might be able to change things. If
> ParallelArray objects are dense by definition, then you don't need the
> holes. I don't think the spec is complete enough to say for sure, but what's
> in there is generally consistent with that notion, so I'd say go for it.
It certainly feels like the right thing to do. See bug 784345 for the patch that implements this, since it affects that bug as well.
(In reply to David Mandelin [:dmandelin] from comment #4)
> Hmmm. To me, it seems like it would be better to just change
> NewDenseCopiedArray so that it just works in this situation. But I'm not
> entirely sure that's the right thing to do--maybe we're relying on the
> assertion inside for testing purposes. I'd ask Jeff but he's on vacation.
> What do you think, Shu?
With the rewrite the version of NewDenseCopiedArray I need is different still -- it both truncates and converts holes to undefineds. So for now I say leave NewDenseCopiedArray and talk to Jeff (I don't think I know him) about this behavior when he's back.
Reporter | ||
Updated•12 years ago
|
Crash Signature: [@ js::HeapSlot::init]
[@ js::NewDenseCopiedArray] → [@ js::HeapSlot::init]
[@ js::NewDenseCopiedArray]
Comment 7•12 years ago
|
||
Comment on attachment 653888 [details] [diff] [review]
fix and testcase
Shu told me this patch is subsumed by the one in bug 784345.
Attachment #653888 -
Flags: review?(dmandelin)
Updated•12 years ago
|
Status: NEW → RESOLVED
Crash Signature: [@ js::HeapSlot::init]
[@ js::NewDenseCopiedArray] → [@ js::HeapSlot::init]
[@ js::NewDenseCopiedArray]
Closed: 12 years ago
Resolution: --- → DUPLICATE
Updated•12 years ago
|
Group: core-security
Reporter | ||
Comment 9•12 years ago
|
||
A testcase for this bug was already added in the original bug (bug 784345).
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•