Bug 100014 – IndexedDB: Bounds check for IDBCursor.advance() incorrect

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joshua Bell]

c/o http://crbug.com/151927

../../third_party/WebKit/Source/WebCore/Modules/indexeddb/IDBCursor.cpp:174:28:error: comparison of constant 9007199254740991 with expression of type 'long' is always false [-Werror,-Wtautological-constant-out-of-range-compare]

  if (count < 1 || count > maxECMAScriptInteger) {

count is a long. This comparison should be against 2^32-1 not 2^53-1

[Hans Wennborg]

Thanks for looking at this!

(In reply to comment #0)
> count is a long. This comparison should be against 2^32-1 not 2^53-1

Should that be 2^31-1?

[Joshua Bell]

Hrm.... this actually gets tricky. The WebIDL in the spec for this is:

http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#cursor

void advance ([EnforceRange] unsigned long count);

And WebIDL says:

http://dev.w3.org/2006/webapi/WebIDL/#es-unsigned-long

"If the conversion to an IDL value is being performed due to ... being passed as an operation argument annotated with the [EnforceRange] extended attribute ... then ... if x < 0 or x > 2^32 − 1, then throw a TypeError"

It would be 2^31-1 if it were a (signed) long. We actually have this marked as |long| in the WebKit IDL to avoid default handling of negative values since [EnforceRange] isn't implemented. Correct behavior of [EnforceRange] is in webkit.org/b/96798 which is blocked on a v8 bug. 

As it stands now, passing 2^32-1 actually fails with a TypeError as well, which is incorrect. This is because V8Binding.cpp's toInt32 conversion for out of bound values returns 0.

So I should probably flip the IDL back to "unsigned long", leave only the 0 test in the code, and add 2^31-1 (ok), 2^31 (ok), 2^32-1 (ok), and 2^32 (type error) as test cases.

[Joshua Bell]

Created attachment 169954 [details]
Patch

[Joshua Bell]

(In reply to comment #2)
> 
> So I should probably flip the IDL back to "unsigned long", leave only the 0 test in the code, and add 2^31-1 (ok), 2^31 (ok), 2^32-1 (ok), and 2^32 (type error) as test cases.

... but I actually had to convert it (temporarily) to "long long" as the binding code's toUint32() conversion for "unsigned long" was not doing the right thing either. *sigh*

[Tony Chang]

Comment on attachment 169954 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169954&action=review

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:173
> +    const unsigned long maxUnsignedLong = 0xffffffff;

Can we use ULONG_MAX or numeric_limits<unsigned long>()?

[WebKit Review Bot]

Comment on attachment 169954 [details]
Patch

Attachment 169954 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14499260

[Joshua Bell]

Created attachment 169976 [details]
Patch

[Joshua Bell]

(In reply to comment #5)
> Can we use ULONG_MAX or numeric_limits<unsigned long>()?

Done (with UINT_MAX - turns out ULONG_MAX ends up being the unsigned long long max...)

[WebKit Review Bot]

Comment on attachment 169976 [details]
Patch

Clearing flags on attachment: 169976

Committed r132140: <http://trac.webkit.org/changeset/132140>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.