Bug 89625 – IndexedDB: Avoid infinite loop if we try to encode -1 for leveldb

• Component: New Bugs
• Status: RESOLVED FIXED

[David Grogan]

IndexedDB: Avoid infinite loop if we try to encode -1 for leveldb

[David Grogan]

Created attachment 148698 [details]
Patch

[David Grogan]

Comment on attachment 148698 [details]
Patch

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

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:142
> +#ifdef NDEBUG

Do any bots run webkit_unit_tests in release mode?

[David Grogan]

Created attachment 148702 [details]
Patch

[David Grogan]

If this passes the ews bots could one of you take a look?

[Alec Flett]

Comment on attachment 148702 [details]
Patch

LGTM - that's neat, I had never seen that use of static_cast, but I found this explanation here:
http://stackoverflow.com/questions/1751346/interpret-signed-as-unsigned

(it's too bad you have to #ifdef NDEBUG the tests though - I can't think of an easy way around it though?)

[Joshua Bell]

The NDEBUG tests make me uneasy. IMHO either the API should support negative values or it shouldn't, and tests should match. 

Is it not possible to change the method signatures to unsigned and get one of the compilers (preferably clang on Linux...) to report an error if you call it with a signed value?

Alternately, the method could be changed to take unsigned and we ASSERT(static_cast<int64_t>(n) >= 0) - then we're catching bad callers (that the compiler SHOULD have complained about, *grumble*) but at least we're not lying in the method signature. The test would still need to be NDEBUG, though.

[David Grogan]

(In reply to comment #6)
> The NDEBUG tests make me uneasy. IMHO either the API should support negative values or it shouldn't, and tests should match.
>
> Is it not possible to change the method signatures to unsigned and get one of the compilers (preferably clang on Linux...) to report an error if you call it with a signed value?

I think this happens already, if the parameter type is changed to uint64_t and you call it with a variable of type int64_t, clang gives a compiler error.

> 
> Alternately, the method could be changed to take unsigned and we ASSERT(static_cast<int64_t>(n) >= 0) - then we're catching bad callers (that the compiler SHOULD have complained about, *grumble*) but at least we're not lying in the method signature. The test would still need to be NDEBUG, though.

I don't understand the difference between this and the idea in the previous paragraph.

In any case, this was my original approach. I have a half-done patch that changes the function to specify a uint64_t parameter and includes ASSERT(n <= INT64_MAX).  That approach required changing the type of a million variables in IDBLevelDBBackingStore and IDBLevelDBCoding from int64_t to uint64_t.

[David Grogan]

Is this patch ok as is?  Any requests before I ask Tony to review?

[Joshua Bell]

(In reply to comment #8)
> Is this patch ok as is?  Any requests before I ask Tony to review?

Yes, sorry.

For the record, we discussed offline and tested. The current C++ compilers we tested (gcc, clang) don't issue a warning if a signed value (e.g. an int64_t) is passed to a function that takes unsigned (e.g. uint64_t). This means that changing the function signature doesn't buy us anything in terms of safety - we'd still want an assert.

[David Grogan]

Tony, could you review this?

[Tony Chang]

Comment on attachment 148702 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:199
> +    ASSERT(nParam >= 0);

The ASSERT suggests that non-positive values are not valid input into the function.  If that's the case, why do we need the defensive coding (the static_cast)?  If there are legitimate ways to call this function with negative numbers, then we should remove the ASSERT.

[David Grogan]

Comment on attachment 148702 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:199
>> +    ASSERT(nParam >= 0);
> 
> The ASSERT suggests that non-positive values are not valid input into the function.  If that's the case, why do we need the defensive coding (the static_cast)?  If there are legitimate ways to call this function with negative numbers, then we should remove the ASSERT.

A call to this function with a negative value indicates a logic error somewhere in the IDB code. Are you saying that means that the static_cast can (and should?) be removed?
uint64_t n = nParam;

[David Grogan]

Some more background:

A call to this function with a negative value indicates a logic error somewhere in the IDB code and would be caught with an ASSERT, in debug mode.

But we've had an undetected logic error that caused -1 to be passed to that function in a release build. An infinite loop followed. The cast to uint64_t just guards against our logic errors causing the thread to enter an infinite loop.

[Tony Chang]

Comment on attachment 148702 [details]
Patch

I think this is OK, but I worry that you're effectively ignoring the logic error in release builds and it could cascade into other logic errors.  Ideally, you would abort or throw an exception or something, but that might not be possible at this point in the code.

Another possibility is to use CRASH() to kill the process in release builds. That has the downside that the user might lose data, but at least we would get a stack trace.

[David Grogan]

Created attachment 149132 [details]
Patch for landing

[David Grogan]

(In reply to comment #14)
> (From update of attachment 148702 [details])
> I think this is OK, but I worry that you're effectively ignoring the logic error in release builds and it could cascade into other logic errors.

Valid concern. As a mitigating factor, in the one scenario we saw this it was part of a cascade from a different error that was obscured by the infinite loop.  So the hope is that that pattern will continue, that this change will make finding errors easier, not harder.

> Ideally, you would abort or throw an exception or something, but that might not be possible at this point in the code.

Good point, I found one place that we could detect this and surface the error easily and added it to the patch.

> Another possibility is to use CRASH() to kill the process in release builds. That has the downside that the user might lose data, but at least we would get a stack trace.

I considered this.  Comparing the scenario of a bad user experience with CRASH() + benefit of getting a stack trace vs scenario of unknown user experience (might even be totally fine) + potentially more difficult debugging, my gut said go with the latter.  Who knows though.

[WebKit Review Bot]

Comment on attachment 149132 [details]
Patch for landing

Rejecting attachment 149132 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
  return self.open(self.click(*args, **kwds))
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 203, in open
    return self._mech_open(url, data, timeout=timeout)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 255, in _mech_open
    raise response
webkitpy.thirdparty.autoinstalled.mechanize._response.httperror_seek_wrapper: HTTP Error 500: Internal Server Error

Full output: http://queues.webkit.org/results/13032561

[David Grogan]

Created attachment 149146 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 149146 [details]
Patch for landing

Clearing flags on attachment: 149146

Committed r121082: <http://trac.webkit.org/changeset/121082>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.