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

RESOLVED FIXED in Firefox 15



DOM: IndexedDB
6 years ago
5 years ago


(Reporter: Jesse Ruderman, Assigned: khuey)


(Blocks: 1 bug, {crash, testcase})

crash, testcase
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox10 unaffected, firefox11 wontfix, firefox12 wontfix, firefox13 affected, firefox14 affected, firefox15 verified)


(crash signature)


(2 attachments, 1 obsolete attachment)



6 years ago
Created attachment 596410 [details]
testcase (crashes Firefox when loaded)

Comment 1

6 years ago
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."
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.


6 years ago
Crash Signature: [TMR] mozilla::dom::indexedDB::Key::EncodeJSVal → [@ array_getElement]
OS: Mac OS X → All
Hardware: x86_64 → All

Comment 3

6 years ago
fyi, automation reproduced this pretty handily on windows and mac all three branches if you need a way to easily retest it.
status-firefox10: --- → unaffected
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
Assignee: nobody → khuey
Created attachment 599327 [details] [diff] [review]
Attachment #599327 - Flags: review?(bent.mozilla)
Note that we might want a MaxRecursionDepth > 5, I just pulled that out of my hat.
Attachment #599327 - Flags: review?(jonas)
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.
Attachment #599327 - Flags: review?(jonas) → review-
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.
Attachment #599327 - Flags: review?(bent.mozilla)
Component: DOM → DOM: IndexedDB
Version: Trunk → unspecified
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.
Attachment #599327 - Attachment is obsolete: true
Attachment #622913 - Flags: review?(jonas)
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.
Attachment #622913 - Flags: review?(jonas) → review+
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
status-firefox11: affected → wontfix
status-firefox12: affected → wontfix
status-firefox14: --- → affected
status-firefox15: --- → fixed


5 years ago
Depends on: 774732
No longer depends on: 774732
No crash loading the testcase.
Verified fixed on FF 15b3 on Win 7/64, Ubuntu 12.04 and Mac OS X 10.6.
status-firefox15: fixed → verified
You need to log in before you can comment on or make changes to this bug.