726376 - Too-much-recursion crash with IndexedDB "cmp" function

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 1

[Jesse Ruderman]

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."

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 3

[Bob Clary [:bc] (inactive)]

fyi, automation reproduced this pretty handily on windows and mac all three branches if you need a way to easily retest it.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Note that we might want a MaxRecursionDepth > 5, I just pulled that out of my hat.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 599327 [details] [diff] [review]
Patch

Review of attachment 599327 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/Key.cpp
@@ +128,5 @@
>  */
>  
>  const int MaxArrayCollapse = 3;
>  
> +const int MaxRecursionDepth = 5;

Make this 100 or so.

@@ +133,5 @@
> +
> +class AutoIncrement
> +{
> +public:
> +  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.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Rather than hardcoding a number let's use JS_CHECK_RECURSION!

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Sicking and I decided that both make sense - hardcode limit of 256 and check recursion stack depth too.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

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 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 622913 [details] [diff] [review]
Patch

Review of attachment 622913 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/IDBFactory.cpp
@@ +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.

::: dom/indexedDB/Key.cpp
@@ +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 @@
> +nsresult
> +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.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/e1af23a8c4e2

Comment 13

[Paul Silaghi, QA [:pauly]]

No crash loading the testcase.
Verified fixed on FF 15b3 on Win 7/64, Ubuntu 12.04 and Mac OS X 10.6.