Last Comment Bug 726376 - Too-much-recursion crash with IndexedDB "cmp" function
: Too-much-recursion crash with IndexedDB "cmp" function
: crash, testcase
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla15
Assigned To: Kyle Huey [:khuey] (
Depends on:
Blocks: 326633
  Show dependency treegraph
Reported: 2012-02-11 18:14 PST by Jesse Ruderman
Modified: 2012-08-06 05:22 PDT (History)
6 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (crashes Firefox when loaded) (86 bytes, text/html)
2012-02-11 18:14 PST, Jesse Ruderman
no flags Details
Patch (7.51 KB, patch)
2012-02-21 13:22 PST, Kyle Huey [:khuey] (
jonas: review-
Details | Diff | Splinter Review
Patch (7.04 KB, patch)
2012-05-10 14:10 PDT, Kyle Huey [:khuey] (
jonas: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-02-11 18:14:44 PST
Created attachment 596410 [details]
testcase (crashes Firefox when loaded)
Comment 1 Jesse Ruderman 2012-02-11 18:16:00 PST
This bug is present in the spec,

"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) PTO Until July 5th 2012-02-11 20:54:44 PST
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:] 2012-02-13 22:49:47 PST
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 [:khuey] ( 2012-02-21 13:22:09 PST
Created attachment 599327 [details] [diff] [review]
Comment 5 Kyle Huey [:khuey] ( 2012-02-22 09:11:41 PST
Note that we might want a MaxRecursionDepth > 5, I just pulled that out of my hat.
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2012-02-22 15:38:21 PST
Comment on attachment 599327 [details] [diff] [review]

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!) 2012-02-23 01:38:24 PST
Rather than hardcoding a number let's use JS_CHECK_RECURSION!
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2012-02-23 02:38:07 PST
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!) 2012-02-23 03:17:25 PST
Sicking and I decided that both make sense - hardcode limit of 256 and check recursion stack depth too.
Comment 10 Kyle Huey [:khuey] ( 2012-05-10 14:10:27 PDT
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 11 Jonas Sicking (:sicking) PTO Until July 5th 2012-05-10 15:29:40 PDT
Comment on attachment 622913 [details] [diff] [review]

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

::: dom/indexedDB/IDBFactory.cpp
@@ +495,3 @@
>    rv = second.SetFromJSVal(aCx, aSecond);

These changes shouldn't be needed, see below.

::: dom/indexedDB/Key.cpp
@@ +139,3 @@
>  {
> +  NS_ENSURE_TRUE(aRecursionDepth < MaxRecursionDepth, 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 [:khuey] ( 2012-05-11 10:49:52 PDT
Comment 13 Paul Silaghi, QA [:pauly] 2012-08-06 05:22:17 PDT
No crash loading the testcase.
Verified fixed on FF 15b3 on Win 7/64, Ubuntu 12.04 and Mac OS X 10.6.

Note You need to log in before you can comment on or make changes to this bug.