Created attachment 596410 [details]
testcase (crashes Firefox when loaded)
This bug is present in the spec, http://www.w3.org/TR/IndexedDB/#dfn-key-1
"Arrays are only valid keys if every item in the array is defined and is a valid key"
"Note that Arrays that contain other Arrays are allowed as valid keys. In this case the algorithm above runs recursively when comparing the individual values in the arrays."
Hrm.. yeah.. Bent was worried about this when he reviewed the patch iirc.
I forgot that it was as easy as creating a cyclic structure and we'd crash. So yeah, we need to add a depth-guard here I suppose. We should probably throw and say that it's not a valid key.
fyi, automation reproduced this pretty handily on windows and mac all three branches if you need a way to easily retest it.
Created attachment 599327 [details] [diff] [review]
Note that we might want a MaxRecursionDepth > 5, I just pulled that out of my hat.
Comment on attachment 599327 [details] [diff] [review]
Review of attachment 599327 [details] [diff] [review]:
@@ +128,5 @@
> const int MaxArrayCollapse = 3;
> +const int MaxRecursionDepth = 5;
Make this 100 or so.
@@ +133,5 @@
> +class AutoIncrement
> + AutoIncrement(PRUint16 *var) : mVar(var)
Why not simply pass the recursion depth as a integer argument to EncodeJSVal. And for each subsequent call pass aCurrentDepth+1. That should result in simpler code and you won't need the guard class.
@@ +239,5 @@
> +Key::DecodeJSValInternal(const unsigned char*& aPos, const unsigned char* aEnd,
> + JSContext* aCx, PRUint8 aTypeOffset, jsval* aVal,
> + PRUint16* aRecursionDepth)
> + AutoIncrement ai(aRecursionDepth);
No need to add any protection for decoding. We know we have encoded a "sane" depth or the encoding would have failed.
Rather than hardcoding a number let's use JS_CHECK_RECURSION!
That means not counting recursion depth, but rather measuring stack size.
The main downside with that is that we could end up with situations where one machine is able to encode very deeply nested arrays and store that in IDB, but when you open the same profile in another machine, it's not able to decode the key since it could have a smaller stack size.
I'd prefer to simply put a hardcoded limit on 100/200/256/500 or some such.
Sicking and I decided that both make sense - hardcode limit of 256 and check recursion stack depth too.
Created attachment 622913 [details] [diff] [review]
Comments addressed, except that I kept the decoding guards in there. The stack depth required could change from version to version, or someone could stick malformed data in a DB. It seems easiest just to leave it there.
Comment on attachment 622913 [details] [diff] [review]
Review of attachment 622913 [details] [diff] [review]:
@@ +495,3 @@
> rv = second.SetFromJSVal(aCx, aSecond);
> + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_DATA_ERR);
These changes shouldn't be needed, see below.
@@ +139,3 @@
> + NS_ENSURE_TRUE(aRecursionDepth < MaxRecursionDepth, NS_ERROR_FAILURE);
> + JS_CHECK_RECURSION(aCx, return NS_ERROR_FAILURE);
I don't really think both of these are needed, but I guess I'm ok with keeping them.
However you should return NS_ERROR_DOM_INDEXEDDB_DATA_ERR in both cases so that functions like IDBObjectStore.put end up throwing the right exception.
@@ +291,5 @@
> +Key::DecodeJSVal(const unsigned char*& aPos, const unsigned char* aEnd,
> + JSContext* aCx, PRUint8 aTypeOffset, jsval* aVal)
> + return DecodeJSValInternal(aPos, aEnd, aCx, aTypeOffset, aVal, 0);
Inline this and the other other Internal method too. Or just use default arguments.
No crash loading the testcase.
Verified fixed on FF 15b3 on Win 7/64, Ubuntu 12.04 and Mac OS X 10.6.