IndexedDB: Avoid infinite loop if we try to encode -1 for leveldb
Created attachment 148698 [details] Patch
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?
Created attachment 148702 [details] Patch
If this passes the ews bots could one of you take a look?
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?)
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.
(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.
Is this patch ok as is? Any requests before I ask Tony to review?
(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.
Tony, could you review this?
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.
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;
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.
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.
Created attachment 149132 [details] Patch for landing
(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.
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
Created attachment 149146 [details] Patch for landing
Comment on attachment 149146 [details] Patch for landing Clearing flags on attachment: 149146 Committed r121082: <http://trac.webkit.org/changeset/121082>
All reviewed patches have been landed. Closing bug.