The following testcase crashes on mozilla-central revision 1ed5a88cd4d0 (run with --fuzzing-safe --ion-eager): function reportCompare (expected) { typeof expected; } var expect = 'No Crash'; var array = new Array(10); for (var i = 0; i != array.length; ++i) { gc(); } var expect = array.length; reportCompare(expect);

Crash trace: Program received signal SIGSEGV, Segmentation fault. check (v=..., this=0x7fffffffc5f0) at js/src/jscntxtinlines.h:89 89 check(v.toString()); #0 check (v=..., this=0x7fffffffc5f0) at js/src/jscntxtinlines.h:89 #1 js::CompartmentChecker::check (this=0x7fffffffc5f0, v=...) at js/src/jscntxtinlines.h:85 #2 0x00000000005e7301 in assertSameCompartment<JS::Rooted<JS::Value> > (t1=..., cx=0x18f08b0) at js/src/jscntxtinlines.h:145 #3 assertSameCompartment<JS::Rooted<JS::Value> > (t1=..., cx=0x18f08b0) at js/src/jsapi.cpp:554 #4 JS_TypeOfValue (cx=0x18f08b0, valueArg=...) at js/src/jsapi.cpp:559 #5 0x00000000007f8234 in js::ion::DoTypeOfFallback (cx=0x18f08b0, frame=0x7fffffffc738, stub=0x18ef8e0, val=..., res=...) at js/src/jit/BaselineIC.cpp:8133 #6 0x00007ffff6bc836b in ?? () #7 0x00007ffff6a3e370 in ?? () Marking s-s and guessing sec-high because GC is involved and this crash signature has previously indicated security problems. Feel free to adjust if this is indeed not exploitable.

JSBugMon: Bisection requested, result: autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: http://hg.mozilla.org/mozilla-central/rev/42776e928f7b user: Eric Faust date: Sat Aug 10 22:20:36 2013 -0700 summary: Bug 902264 - Part 2: Expose Array.length optimization to idempotent GetPropertyICs. (r=jandem) This iteration took 357.511 seconds to run.

Needinfo from Eric based on comment 3.

OK, so, here's the problem: We didn't used to emit Array.length stubs for idempotent caches. This allowed us to invalidate the top ion script, where the fact that |array.length| was an int was propagated through TI. Since now we do emit a length stub, we don't get that invalidation, hence the confusion when we bail to baseline and construct a value with a "known" (but erroneous) type and try to pass it around. This seems like it's a problem not with the ICs, but with the data we know about Array.length. TI tracks whether an object is an array, and the length property is not configurable, which means we can just statically know that if we see length on an array type, that the result type set of that operation is AT LEAST Double and Int. Brian, does this seem accurate? Otherwise, I /guess/ we can mask Array.length on idempotent caches again, but it seems nuts that we can do it in parallel but not in idempotent sequential.

I'm not following this explanation. Eric, can you elaborate on paragraph 2 and what the erroneous value looks like and how that leads to the crash? TI doesn't track the types of array lengths in the property type sets, as the |length| property of an array isn't a slotful one, but does keep track of whether arrays might have an overflowing length, via OBJECT_FLAG_LENGTH_OVERFLOW. Ion checks this when inlining array.length accesses, but if the length is accessed via an IC the overflowing case needs to be accounted for. In particular, the array.length IC stub should only be generated if the observed types contain int32, and if the observed types do not contain double then the stub needs to check for an overflowing length.

(In reply to Brian Hackett (:bhackett) from comment #6) > I'm not following this explanation. Eric, can you elaborate on paragraph 2 > and what the erroneous value looks like and how that leads to the crash? Sure. We say in Ion that the argument to the call of |reportCompare| is always a string, so when we bail to baseline, we get a value with a payload of 10 (array.length just as expected) and a tag of JSVAL_TYPE_STRING. There just isn't any TI saying it could be int at all. If we had invalidated and run the instruction over again where it would be monitored, then we would obviously never generate ion code with the string specialization again. I guess the part that bothers me is that I feel like we should be able to statically know the types that can be in |expect| because the string is hardcoded, and array.length, slotful or not, can only have one of two things. If we need to check for overflow, that's actually OK, but not being able to generate an IC stub in this case seems really strange to me.

(In reply to Eric Faust [:efaust] from comment #7) > (In reply to Brian Hackett (:bhackett) from comment #6) > > I'm not following this explanation. Eric, can you elaborate on paragraph 2 > > and what the erroneous value looks like and how that leads to the crash? > > Sure. We say in Ion that the argument to the call of |reportCompare| is > always a string, so when we bail to baseline, we get a value with a payload > of 10 (array.length just as expected) and a tag of JSVAL_TYPE_STRING. There > just isn't any TI saying it could be int at all. If we had invalidated and > run the instruction over again where it would be monitored, then we would > obviously never generate ion code with the string specialization again. > > I guess the part that bothers me is that I feel like we should be able to > statically know the types that can be in |expect| because the string is > hardcoded, and array.length, slotful or not, can only have one of two > things. If we need to check for overflow, that's actually OK, but not being > able to generate an IC stub in this case seems really strange to me. OK, the crash makes sense to me now. As you describe it, the problem seems to be what I outlined above for when it is OK for Ion to generate an Array.length stub. I would suppose that in the last compilation Ion thinks the value of |array.length| in 'var expect = array.length' has an empty type set, and won't require a barrier in the write to |expect|. That Ion allowed that integer value to be produced without the type set being updated is a bug, and if the observed type set for |array.length| was updated when the expression actually executed then the Ion code would be invalidated and the remaining code would execute correctly. I wouldn't worry about what we should or shouldn't statically know here. These --ion-eager fuzz bugs are really pretty irrelevant to assessing the quality of the generated code, and it's really just about making sure that we are preserving the correct invariants. In this case, Ion code can't generate a value for a MIR node which is not captured by that MIR node's type, with the MIR node in this case being the IC for |array.length|.

Comment on attachment 792475 [details] [diff] [review] Fix Review of attachment 792475 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonCaches.cpp @@ +1170,5 @@ > + > + // Since we always generate non-overflowing results and cannot monitor > + // the results, we should be OK as long as we already know about the int > + // return. > + return propTypes->hasType(types::Type::Int32Type()); It's not appropriate to look at the property type sets, as they aren't required to reflect values of the |length| property. I think they do right now, but that's more accidental than anything else and could change in the future. Instead, you should check here that the observed type sets for the pc associated with the cache (which may require a separate bit on the cache if it is idempotent and reflects multiple pcs) contain int32, and the |length| stub should always check for an overflowing length.

The length stub needn't check for overflowing length, since the stub cannot produce one. We only generate outputs of type Int32.

Here's another test (without gc) that might be the same issue. Posting it here so efaust can verify it's indeed the same bug once he has a patch. If it turns out to be a different issue, then we'll clone this into a new bug: y = "x"; function test() { var a = []; var x = 0; for (x = -(2 + a.length); x <= (2 + a.length); x++) for (y = (2 + a.length); y >= -(2 + a.length); y--) {} } test();

This isn't fully generalized, but it will do for now. If runtimeData_ realloc()s to account for an append, any pointers into it will be stale. Use this wrapper instead.

I've seen various instances of this problem now, including object addresses being fully controllable through Array.length, so I'm marking this sec-critical.

Comment on attachment 794998 [details] [diff] [review] Implement IonCacheReference to handle stale pointers in addCache() Review of attachment 794998 [details] [diff] [review]: ----------------------------------------------------------------- This is good, but a bit of alpha-renaming and making it more generic would help people thinking that they can reuse this :) ::: js/src/jit/shared/CodeGenerator-shared.h @@ +220,5 @@ > + public: > + IonCacheReference(CodeGeneratorShared *cg, size_t index) > + : cg_(cg), index_(index) { } > + > + IonCache * operator ->() { Ok, this is just weird to have a class named *Reference and to have a -> operator on it. As discuss in person, having a template to wrap any data in the runtime data list would be good, so using something like DataPtr<IonCache> cache(this, cacheIndex); would make more sense. The only trick is that we should change the index returned by allocateCache to be the dataOffset instead of the index of the cacheList. The index of the cacheList is only useful during GCs for doing a reset of all caches. This way we can get rid of the cacheList_[index_] and make this one generic. @@ +229,5 @@ > + } > + }; > + > + IonCacheReference getCache(size_t index) { > + return IonCacheReference(this, index); I think we can remove this function and directly call the constructor.

Comment on attachment 794999 [details] [diff] [review] Implement InlineConcatLists, without sentinel nodes, for union-style algorithms. Review of attachment 794999 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/InlineList.h @@ +392,5 @@ > > +/* This list type is more or less exactly an InlineForwardList without a sentinel > + * node. It is useful in cases where you are doing algorithms that deal with many > + * merging singleton lists, rather than often empty ones. > + */ nit: Use C++ comment style. @@ +396,5 @@ > + */ > +template <typename T> class InlineConcatListIterator; > +template <typename T> > +class InlineConcatList > +{ Why not inheriting from InlineListNode<T> instead? [answer & r=>?] @@ +415,5 @@ > + return iterator(this); > + } > + > + iterator end() { > + return iterator(NULL); Replace NULL by tail->next. This way an InlineConcatList can be append in something else once but also, it would still be possible to iterate over it independently. @@ +427,5 @@ > + JS_ASSERT(!adding->tail->next); > + > + tail->next = adding; > + tail = adding->tail; > + adding->tail = NULL; With the previous modification, you can just remove this last line. @@ +433,5 @@ > + > + protected: > + friend class InlineConcatListIterator<T>; > + Node *next; > + Node *tail; nit: next_ & tail_ @@ +472,5 @@ > + return iter == where.iter; > + } > + > + private: > + Node *iter; nit: iter_

Comment on attachment 795000 [details] [diff] [review] Route code location information from GVN though MGetPropertyCache Review of attachment 795000 [details] [diff] [review]: ----------------------------------------------------------------- Nice.

Comment on attachment 794999 [details] [diff] [review] Implement InlineConcatLists, without sentinel nodes, for union-style algorithms. (In reply to Nicolas B. Pierron [:nbp] from comment #22) > @@ +396,5 @@ > > + */ > > +template <typename T> class InlineConcatListIterator; > > +template <typename T> > > +class InlineConcatList > > +{ > > Why not inheriting from InlineListNode<T> instead? [answer & r=>?] I hope this is what "[answer and r=>?]" meant. I...suppose we could do that. There's no absurdly good reason not to do it. It would be just as well to implement it on top of InlineListNode<T>, using |prev| to mean |tail| (which I think is less clear), and hacking an inheritance of InlineListIterator<T> to be exactly the same without exposing operator-- (because prev means tail now). I just thought this was slightly unsightly and a little bit unclear. I guess I'm not completely opposed to it. I did notice at the time that InlineConcatListIterator and InlineListIterator were nearly identical, but either way, we need to hide --, so there's going to either be a lot of |using| or code copy.

(In reply to Nicolas B. Pierron [:nbp] from comment #21) > Comment on attachment 794998 [details] [diff] [review] > Implement IonCacheReference to handle stale pointers in addCache() > > Review of attachment 794998 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is good, but a bit of alpha-renaming and making it more generic would > help people thinking that they can reuse this :) > > ::: js/src/jit/shared/CodeGenerator-shared.h > @@ +220,5 @@ > > + public: > > + IonCacheReference(CodeGeneratorShared *cg, size_t index) > > + : cg_(cg), index_(index) { } > > + > > + IonCache * operator ->() { > > Ok, this is just weird to have a class named *Reference and to have a -> > operator on it. > As discuss in person, having a template to wrap any data in the runtime data > list would be good, so using something like > > DataPtr<IonCache> cache(this, cacheIndex); > > would make more sense. > > The only trick is that we should change the index returned by allocateCache > to be the dataOffset instead of the index of the cacheList. The index of > the cacheList is only useful during GCs for doing a reset of all caches. I don't think this is actually true. Reading the code, this is the way that the IC is looked up at runtime and passed to IC::update(). allocateCache() flows directly into addCache() which creates the OutOfLineUpdateCache, which takes the index for pushing in all of the stubs. I think the best thing to do here is move to DataPtr, as you suggest, but leave the cacheIndex, and getCache(), which does the lookup once, and returns a DataPtr, as requested.

(In reply to Eric Faust [:efaust] from comment #24) > Comment on attachment 794999 [details] [diff] [review] > Implement InlineConcatLists, without sentinel nodes, for union-style > algorithms. > > (In reply to Nicolas B. Pierron [:nbp] from comment #22) > > @@ +396,5 @@ > > > + */ > > > +template <typename T> class InlineConcatListIterator; > > > +template <typename T> > > > +class InlineConcatList > > > +{ > > > > Why not inheriting from InlineListNode<T> instead? [answer & r=>?] > > I hope this is what "[answer and r=>?]" meant. I...suppose we could do that. > There's no absurdly good reason not to do it. It would be just as well to > implement it on top of InlineListNode<T>, using |prev| to mean |tail| (which > I think is less clear), and hacking an inheritance of InlineListIterator<T> > to be exactly the same without exposing operator-- (because prev means tail > now). I just thought this was slightly unsightly and a little bit unclear. > > I guess I'm not completely opposed to it. I did notice at the time that > InlineConcatListIterator and InlineListIterator were nearly identical, but > either way, we need to hide --, so there's going to either be a lot of > |using| or code copy. It occurs to me that you might have just meant "why not make it doubly linked and inherit". We could do this with a similar, very careful circular list, and merge them appropriately. It would be no bigger, just more complex. Is this what you meant?

Move to generic DataPtr, leaving getCache() as per my above comment.

Updated in light of DataPtr fix.

Comment on attachment 795820 [details] [diff] [review] DataPtr.patch Review of attachment 795820 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/shared/CodeGenerator-shared.h @@ +222,5 @@ > + DataPtr(CodeGeneratorShared *cg, size_t index) > + : cg_(cg), index_(index) { } > + > + T * operator ->() { > + return lookup(); We need to be careful here, as this code suffer from the same problem we had with rooted. Which is that the "this" pointer for the next method call might become invalid if under some function under the method appending any data to the runtimeData vector which cause it to be resized. I do not expect this kind of issue to appear in a near future, but I just want to make sure we are aware of it.

(In reply to Nicolas B. Pierron [:nbp] from comment #29) > We need to be careful here, as this code suffer from the same problem we had > with rooted. > Which is that the "this" pointer for the next method call might become > invalid if under some function under the method appending any data to the > runtimeData vector which cause it to be resized. > > I do not expect this kind of issue to appear in a near future, but I just > want to make sure we are aware of it. Is it possible and does it make sense to add an assertion for that?

(In reply to Eric Faust [:efaust] from comment #26) > > (In reply to Nicolas B. Pierron [:nbp] from comment #22) > > > Why not inheriting from InlineListNode<T> instead? [answer & r=>?] > > It occurs to me that you might have just meant "why not make it doubly > linked and inherit". We could do this with a similar, very careful circular > list, and merge them appropriately. It would be no bigger, just more > complex. Is this what you meant? No I just meant why not reusing the underlying structure, knowing that we already have it.

(In reply to Christian Holler (:decoder) from comment #30) > (In reply to Nicolas B. Pierron [:nbp] from comment #29) > > > We need to be careful here, as this code suffer from the same problem we had > > with rooted. > > Which is that the "this" pointer for the next method call might become > > invalid if under some function under the method appending any data to the > > runtimeData vector which cause it to be resized. > > > > I do not expect this kind of issue to appear in a near future, but I just > > want to make sure we are aware of it. > > > Is it possible and does it make sense to add an assertion for that? Sadly C++ does not provide any good mean to assert around the function boundaries. One complex way would be to poisoned and flag the memory in valgrind once vector is moved inside the LifoAlloc.

Comment on attachment 795825 [details] [diff] [review] Route code location information from MGetPropertyCache to GetPropertyIC and IonScript runtimeData storage Review of attachment 795825 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/CodeGenerator.cpp @@ +104,3 @@ > cache->setIdempotent(); > + if (cache->isGetProperty()) > + addCacheLocations(cache, mir->toGetPropertyCache()); Move this to visitGetPropertyIC. ::: js/src/jit/IonCaches.h @@ +502,5 @@ > +// Helper for idempotent GetPropertyIC location tracking. Declared externally > +// to be forward declarable. > +struct CacheLocation { > + jsbytecode *pc; > + JSScript *script; Add a comment that to mention that this class is only attached to the IonCaches and that it is safe even if we do not have a HeapPtrScript because all the scripts listed here are marked as part of the IonCode holding this IC. ::: js/src/jit/shared/CodeGenerator-shared.cpp @@ +903,5 @@ > } > } > > +void > +CodeGeneratorShared::addCacheLocations(DataPtr<IonCache> &cache, MGetPropertyCache *mir) Rename this function to: size_t allocateLocations(const CacheLocationList &locs, size_t *numLocations); such as it does not depend on the MIR instruction being used. @@ +911,5 @@ > + CacheLocationList &locs = mir->location(); > + for (CacheLocationList::iterator iter = locs.begin(); iter != locs.end(); iter++) { > + size_t curIndex = runtimeData_.length(); > + masm.propagateOOM( > + runtimeData_.appendN(0, sizeof(mozilla::AlignedStorage2<CacheLocation>))); use: curIndex = allocateData(sizeof(CacheLocation)); @@ +917,5 @@ > + return; > + new (&runtimeData_[curIndex]) CacheLocation(iter->pc, iter->script); > + numLocations++; > + } > + cache->toGetProperty().setLocationInfo(firstIndex, numLocations); This line should move to the visitGetPropertyIC function and numLocations can be and outParam of the allocateLocations function.

Splitting all this DataPtr discussing off to bug 909989, as none of it is security critical.

Applies on top of work ongoing in bug 909989

Comment on attachment 796417 [details] [diff] [review] Route code location information to GetPropertyIC via runtimeData Review of attachment 796417 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonCaches.h @@ +502,5 @@ > +// Helper for idempotent GetPropertyIC location tracking. Declared externally > +// to be forward declarable. > +struct CacheLocation { > + jsbytecode *pc; > + JSScript *script; Add the comment about HeapPtrScript that I mentionned earlier in comment 33. ::: js/src/jit/shared/CodeGenerator-shared.cpp @@ +908,5 @@ > +{ > + size_t firstIndex = runtimeData_.length(); > + size_t numLocations = 0; > + for (CacheLocationList::iterator iter = locs.begin(); iter != locs.end(); iter++) { > + size_t curIndex = allocateData(sizeof(CacheLocation)); After the loop, you will need to call allocateData to add some padding, such as if the CacheLocation structure change, we can still ensure the alignment. Or you can assert and comment that we would need to do that if the size of the CacheLocation structure does not imply that data are already aligned.

An interdiff in light of comment 36. Applies on top of attachment 796417 [details] [diff] [review]. If r+, that should =>r+ as well.

Comment on attachment 796828 [details] [diff] [review] Address nits Review of attachment 796828 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonCaches.h @@ +504,5 @@ > +// > +// Since all the scripts stored in CacheLocations are guaranteed to have been > +// Ion compiled, and are kept alive by function objects in jitcode, and since > +// the CacheLocations only have the lifespan of the jitcode, there is no need > +// for HeapPtrs here, since they won't be GC'd, and scripts are always tenured. nit: since they won't be GC'd --> since they won't be moved. ::: js/src/jit/shared/CodeGenerator-shared.cpp @@ +914,5 @@ > size_t curIndex = allocateData(sizeof(CacheLocation)); > new (&runtimeData_[curIndex]) CacheLocation(iter->pc, iter->script); > numLocations++; > } > JS_ASSERT(numLocations != 0); Assert here that the length of the runtime data is still aligned.

Comment on attachment 796417 [details] [diff] [review] Route code location information to GetPropertyIC via runtimeData Review of attachment 796417 [details] [diff] [review]: ----------------------------------------------------------------- r+ with and fold with following patch nits.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7779522f6f6 https://hg.mozilla.org/integration/mozilla-inbound/rev/f66ece725f98 https://hg.mozilla.org/integration/mozilla-inbound/rev/a95e00dbefea https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb7a1c950a3

https://hg.mozilla.org/mozilla-central/rev/a5bac058df4f https://hg.mozilla.org/mozilla-central/rev/0c9ef3daeb0d https://hg.mozilla.org/mozilla-central/rev/cb78e376db2b https://hg.mozilla.org/mozilla-central/rev/0b408255c923

JSBugMon: This bug has been automatically verified fixed.