Bug 165978

Created attachment 297376 [details] Patch v1

Comment on attachment 297376 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=297376&action=review > Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:222 > + m_currentLowerKey = m_fetchedRecords.first().record.key; All the calls to Deque::first except these two have assertions that the deque is not empty. Deque::first has an internal assumption that it's not empty. Should you add more assertions here or remove the others? > Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:317 > + for (size_t i = 0; i < count && !m_fetchedRecords.isEmpty(); ++i) { > + if (m_fetchedRecords.first().isTerminalRecord()) I don't see any bounds checking on the value of count other than making sure it's nonzero. Are there tests that try to advance the cursor more than it can or put a negative/non-integer value into IDBCursor.advance? I don't see any guarantee that this will continue to have a first value for the entire duration of this for loop. > Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:350 > + ASSERT(m_fetchedRecords.isEmpty() || !m_fetchedRecords.last().isTerminalRecord()); If m_fetchedRecords is empty, m_fetchedRecords.last() will assert and read memory it shouldn't

(In reply to comment #2) > Comment on attachment 297376 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=297376&action=review > > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:222 > > + m_currentLowerKey = m_fetchedRecords.first().record.key; > > All the calls to Deque::first except these two have assertions that the > deque is not empty. Deque::first has an internal assumption that it's not > empty. Should you add more assertions here or remove the others? Some places that don't have ASSERTs *do* have "if (!deque.isEmpty())" checks beforehand. Each site that calls to first() should have *either* an ASSERT or an appropriate isEmpty() check. I will go through and make sure they all have at least one of those. > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:317 > > + for (size_t i = 0; i < count && !m_fetchedRecords.isEmpty(); ++i) { > > + if (m_fetchedRecords.first().isTerminalRecord()) > > I don't see any bounds checking on the value of count other than making sure > it's nonzero. Are there tests that try to advance the cursor more than it > can or put a negative/non-integer value into IDBCursor.advance? Yes. Negative counts fail at the bindings level and "advance more than it can" is spec'ed behavior. e.g., if you advance 50 and there's only 4 left, you simply reach the end. > I don't see any guarantee that this will continue to have a first value for the entire > duration of this for loop. The guarantee is in the for clause: ` i < count && !m_fetchedRecords.isEmpty();` By making non-emptiness of the deque an invariant of the loop. > > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:350 > > + ASSERT(m_fetchedRecords.isEmpty() || !m_fetchedRecords.last().isTerminalRecord()); > > If m_fetchedRecords is empty, m_fetchedRecords.last() will assert and read > memory it shouldn't Agreed, but this assertion never does that; If the m_fetchedRecords.last() code runs, the deque is definitely not empty.

Created attachment 297391 [details] Patch

Comment on attachment 297391 [details] Patch Clearing flags on attachment: 297391 Committed r209960: <http://trac.webkit.org/changeset/209960>

All reviewed patches have been landed. Closing bug.