Closed Bug 784342 Opened 12 years ago Closed 12 years ago

Crash [@ js::HeapSlot::init]

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED DUPLICATE of bug 784345

People

(Reporter: decoder, Unassigned)

References

Details

(Keywords: crash, testcase, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(1 file)

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.
Crash Signature: [@ js::HeapSlot::init] [@ js::NewDenseCopiedArray] → [@ js::HeapSlot::init] [@ js::NewDenseCopiedArray]
Attached patch fix and testcaseSplinter Review
Attachment #653888 - Flags: review?(dmandelin)
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 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
(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.
(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.
Crash Signature: [@ js::HeapSlot::init] [@ js::NewDenseCopiedArray] → [@ js::HeapSlot::init] [@ js::NewDenseCopiedArray]
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)
Status: NEW → RESOLVED
Crash Signature: [@ js::HeapSlot::init] [@ js::NewDenseCopiedArray] → [@ js::HeapSlot::init] [@ js::NewDenseCopiedArray]
Closed: 12 years ago
Resolution: --- → DUPLICATE
Group: core-security
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.

Attachment

General

Created:
Updated:
Size: