New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 594618 link

Starred by 1 user

Issue metadata

Status: Duplicate
Merged: issue 595792
Owner:
ex-Googler
Closed: Mar 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

Out of bounds access in sqlite3ExprSkipCollate.

Project Member Reported by reillyg@chromium.org, Mar 14 2016

Issue description

Dr. Memory has detected a stack overflow within sqlite3ExprSkipCollate:

https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%28DrMemory%20full%29%20%281%29/builds/4409

UNADDRESSABLE ACCESS beyond top of stack: reading 0x10f6f2f8-0x10f6f2fc 4 byte(s)
# 0 chromium_sqlite3.dll!sqlite3ExprSkipCollate                                [third_party\sqlite\amalgamation\sqlite3.c:86121]
# 1 chromium_sqlite3.dll!isDistinctRedundant                                   [third_party\sqlite\amalgamation\sqlite3.c:123159]
# 2 chromium_sqlite3.dll!sqlite3WhereBegin                                     [third_party\sqlite\amalgamation\sqlite3.c:126863]
# 3 chromium_sqlite3.dll!sqlite3Select                                         [third_party\sqlite\amalgamation\sqlite3.c:114915]
# 4 chromium_sqlite3.dll!yy_reduce                                             [third_party\sqlite\amalgamation\sqlite3.c:129801]
# 5 chromium_sqlite3.dll!sqlite3Parser                                         [third_party\sqlite\amalgamation\sqlite3.c:130886]
# 6 chromium_sqlite3.dll!sqlite3RunParser                                      [third_party\sqlite\amalgamation\sqlite3.c:131731]
# 7 chromium_sqlite3.dll!sqlite3Prepare                                        [third_party\sqlite\amalgamation\sqlite3.c:109524]
# 8 chromium_sqlite3.dll!sqlite3LockAndPrepare                                 [third_party\sqlite\amalgamation\sqlite3.c:109619]
# 9 chromium_sqlite3.dll!sqlite3_prepare_v2                                    [third_party\sqlite\amalgamation\sqlite3.c:109695]
#10 sql.dll!sql::Connection::GetUniqueStatement                                [sql\connection.cc:1477]
#11 net::SQLitePersistentCookieStore::Backend::InitializeDatabase              [net\extras\sqlite\sqlite_persistent_cookie_store.cc:650]
#12 net::SQLitePersistentCookieStore::Backend::LoadAndNotifyInBackground       [net\extras\sqlite\sqlite_persistent_cookie_store.cc:476]
#13 base::internal::Invoker<>::Run                                             [base\bind_internal.h:352]
#14 base.dll!base::SequencedWorkerPool::Inner::ThreadLoop                      [base\threading\sequenced_worker_pool.cc:834]
#15 base.dll!base::SequencedWorkerPool::Worker::Run                            [base\threading\sequenced_worker_pool.cc:535]
#16 base.dll!base::SimpleThread::ThreadMain                                    [base\threading\simple_thread.cc:66]
#17 base.dll!base::`anonymous namespace'::ThreadFunc                           [base\threading\platform_thread_win.cc:84]
#18 KERNEL32.dll!BaseThreadInitThunk                                          +0x11     (0x76cf337a <KERNEL32.dll+0x1337a>)
Note: @0:03:05.451 in thread 2856
Note: 0x10f6f2f8 refers to -8 byte(s) beyond the top of the stack 0x10f6f2f0
Note: instruction: mov    0x08(%ebp) -> %eax
Suppression (error hash=#3DC559BF2B99D528#):
For more info on using suppressions see http://dev.chromium.org/developers/how-tos/using-drmemory#TOC-Suppressing-error-reports-from-the-
{
UNADDRESSABLE ACCESS
name=<insert_a_suppression_name_here>
chromium_sqlite3.dll!sqlite3ExprSkipCollate
chromium_sqlite3.dll!isDistinctRedundant
chromium_sqlite3.dll!sqlite3WhereBegin
chromium_sqlite3.dll!sqlite3Select
chromium_sqlite3.dll!yy_reduce
chromium_sqlite3.dll!sqlite3Parser
chromium_sqlite3.dll!sqlite3RunParser
chromium_sqlite3.dll!sqlite3Prepare
chromium_sqlite3.dll!sqlite3LockAndPrepare
chromium_sqlite3.dll!sqlite3_prepare_v2
sql.dll!sql::Connection::GetUniqueStatement
*!net::SQLitePersistentCookieStore::Backend::InitializeDatabase
*!net::SQLitePersistentCookieStore::Backend::LoadAndNotifyInBackground
*!base::internal::Invoker<>::Run
base.dll!base::SequencedWorkerPool::Inner::ThreadLoop
base.dll!base::SequencedWorkerPool::Worker::Run
base.dll!base::SimpleThread::ThreadMain
base.dll!base::`anonymous namespace'::ThreadFunc
KERNEL32.dll!BaseThreadInitThunk
}
 

Comment 1 by mmenke@chromium.org, Mar 14 2016

Are we using the same operating definition of stack overflow?  That callstack looks pretty short for a stack overflow, and we aren't doing anything weird like using alloca, though I suppose sqlite might be.
Cc: sh...@chromium.org
Summary: Out of bounds access in sqlite3ExprSkipCollate. (was: Stack overflow in sqlite3ExprSkipCollate.)
Apologies, I meant "access beyond the top of the stack."

There are actually a number of failures with similar SQLite stack traces coming from AppCache as well so I think this is a recently introduced SQLite issue:

UNADDRESSABLE ACCESS beyond top of stack: reading 0x0038f264-0x0038f268 4 byte(s)
# 0 chromium_sqlite3.dll!sqlite3ExprSkipCollate                                [third_party\sqlite\amalgamation\sqlite3.c:86121]
# 1 chromium_sqlite3.dll!isDistinctRedundant                                   [third_party\sqlite\amalgamation\sqlite3.c:123159]
# 2 chromium_sqlite3.dll!sqlite3WhereBegin                                     [third_party\sqlite\amalgamation\sqlite3.c:126863]
# 3 chromium_sqlite3.dll!sqlite3Select                                         [third_party\sqlite\amalgamation\sqlite3.c:114915]
# 4 chromium_sqlite3.dll!yy_reduce                                             [third_party\sqlite\amalgamation\sqlite3.c:129801]
# 5 chromium_sqlite3.dll!sqlite3Parser                                         [third_party\sqlite\amalgamation\sqlite3.c:130886]
# 6 chromium_sqlite3.dll!sqlite3RunParser                                      [third_party\sqlite\amalgamation\sqlite3.c:131731]
# 7 chromium_sqlite3.dll!sqlite3Prepare                                        [third_party\sqlite\amalgamation\sqlite3.c:109524]
# 8 chromium_sqlite3.dll!sqlite3LockAndPrepare                                 [third_party\sqlite\amalgamation\sqlite3.c:109619]
# 9 chromium_sqlite3.dll!sqlite3_prepare_v2                                    [third_party\sqlite\amalgamation\sqlite3.c:109695]
#10 sql.dll!sql::Connection::GetUniqueStatement                                [sql\connection.cc:1477]
#11 content.dll!content::AppCacheDatabase::FindOriginsWithGroups               [content\browser\appcache\appcache_database.cc:257]
#12 content.dll!content::AppCacheDatabase::GetAllOriginUsage                   [content\browser\appcache\appcache_database.cc:240]
#13 content::AppCacheDatabaseTest_WasCorrutionDetected_Test::TestBody          [content\browser\appcache\appcache_database_unittest.cc:144]
#14 testing::internal::HandleExceptionsInMethodIfSupported<>                   [testing\gtest\src\gtest.cc:2458]
Note: @0:00:42.839 in thread 1080
Note: 0x0038f264 refers to -8 byte(s) beyond the top of the stack 0x0038f25c
Note: instruction: mov    0x08(%ebp) -> %eax
Suppression (error hash=#432097A39ABD422A#):
For more info on using suppressions see http://dev.chromium.org/developers/how-tos/using-drmemory#TOC-Suppressing-error-reports-from-the-
{
UNADDRESSABLE ACCESS
name=<insert_a_suppression_name_here>
chromium_sqlite3.dll!sqlite3ExprSkipCollate
chromium_sqlite3.dll!isDistinctRedundant
chromium_sqlite3.dll!sqlite3WhereBegin
chromium_sqlite3.dll!sqlite3Select
chromium_sqlite3.dll!yy_reduce
chromium_sqlite3.dll!sqlite3Parser
chromium_sqlite3.dll!sqlite3RunParser
chromium_sqlite3.dll!sqlite3Prepare
chromium_sqlite3.dll!sqlite3LockAndPrepare
chromium_sqlite3.dll!sqlite3_prepare_v2
sql.dll!sql::Connection::GetUniqueStatement
content.dll!content::AppCacheDatabase::FindOriginsWithGroups
content.dll!content::AppCacheDatabase::GetAllOriginUsage
*!content::AppCacheDatabaseTest_WasCorrutionDetected_Test::TestBody
*!testing::internal::HandleExceptionsInMethodIfSupported<>
}

Adding shess@.
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/664bf5cfb36ba93d714acecfd197bf76e887fd1c

commit 664bf5cfb36ba93d714acecfd197bf76e887fd1c
Author: reillyg <reillyg@chromium.org>
Date: Mon Mar 14 18:11:52 2016

Add suppressions for new Windows Unit Dr. Memory failures.

BUG= 594614 , 594618 
TBR=thestig@chromium.org
NOTRY=true

Review URL: https://codereview.chromium.org/1795303003

Cr-Commit-Position: refs/heads/master@{#381009}

[modify] https://crrev.com/664bf5cfb36ba93d714acecfd197bf76e887fd1c/tools/valgrind/drmemory/suppressions_full.txt

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc6c73c5c7650836662b7b04224362e14a273741

commit fc6c73c5c7650836662b7b04224362e14a273741
Author: reillyg <reillyg@chromium.org>
Date: Mon Mar 14 18:25:42 2016

Generalize SQLite Dr. Memory suppression.

These errors have been seen coming from more modules than just cookie
storage.

BUG= 594618 
TBR=thestig@chromium.org
NOTRY=true

Review URL: https://codereview.chromium.org/1801473004

Cr-Commit-Position: refs/heads/master@{#381017}

[modify] https://crrev.com/fc6c73c5c7650836662b7b04224362e14a273741/tools/valgrind/drmemory/suppressions_full.txt

Comment 5 by mmenke@chromium.org, Mar 14 2016

Cc: michaeln@chromium.org
[+michaeln], the other sqlite3 owner, though looks like shess has been more active there, lately.

Comment 6 by eroman@chromium.org, Mar 15 2016

This may also be related to  issue 594614 , which started in the same regression window. Both failing tests run fairly near to each other.

Comment 7 by mmenke@chromium.org, Mar 15 2016

And since this error affects more test fixtures than just net/, this seems more likely to be the underlying bug than the other issue.

Comment 8 by sh...@chromium.org, Mar 15 2016

IMHO, this is from the VS 2015 changeover, so it's likely to be a latent problem caused by new code generation rather than a new problem caused by a particular CL.  [That doesn't mean it shouldn't be fixed.]

Speaking to the SQLite changes directly - I don't think there's been anything remotely related to this area of SQLite changed in a few months.  Certainly the most-recent changes wouldn't affect this area, unless there's some sort of broad optimization change happening.

[Also, I was OOT for a long weekend starting Friday.  I'll look at it tomorrow.]

Comment 9 by sh...@chromium.org, Mar 16 2016

Owner: sh...@chromium.org
Status: Started (was: Untriaged)
So far, nothing to report.  SQLite has options to use alloca, but is not using alloca.

SQLite does sometimes use interesting malloc strategies.  For instance, struct Expr has "ALLOCATION NOTES" which describe how under EP_Reduced and EP_TokenOnly (set in Expr.flags), the structure is truncated.  Furthermore, it can do a bulk allocation for a tree of Expr children.  I haven't been able to really figure out if this is relevant to the problem, though.

[This kind of shenanigans does seem questionable WRT compiler optimizations, though.  For instance, in this case the compiler may not realize that the ExprHasProperty() check is a guard against accessing pExpr->pLeft, if it's trying to unroll the loop to check whether pExpr->pLeft!=NULL in parallel with ExprHasProperty(pExpr, EP_Skip).]

Comment 10 by sh...@chromium.org, Mar 17 2016

Status: Assigned (was: Started)
Still no joy.  AFAICT, the read in question shouldn't even be referencing stack data, it should be reading Expr.flags from a heap allocation.

Both the cookie and appcache case involve a query for DISTINCT data from an indexed column where the index is not unique.  I can't tell if that is real info, though, since that's the only case where isDistinctRedundant() would be on the stack in the first place.  Of course the call is going to return false in this case.  AFAICT, there are no TK_COLLATE operators involved with this query.  This is sensible, as the underlying index is already collated as appropriate, so the DISTINCT pass can rely on the duplicates coming in a run of one or more at once.  There's only one item in the expression array, the column itself, a single-node tree with flags EP_NoReduce|EP_Resolved, so the loop in sqlite3ExprSkipCollate() should never be entered.  The other DISTINCT cases I find in the codebase aren't as simple.
Mergedinto: 595792
Status: Duplicate (was: Assigned)
Note the bogus invalid offset ("refers to -8 byte(s) beyond the top of the stack").  This is a false positive coming from the same Dr. Memory bug as  issue 595792 .  Apologies for the confusion: this bug manifested as several seemingly different symptoms on the surface.
Project Member

Comment 12 by bugdroid1@chromium.org, Mar 20 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/899620623be8f7b5004cea13f7da96ff7ca80c27

commit 899620623be8f7b5004cea13f7da96ff7ca80c27
Author: bruening <bruening@chromium.org>
Date: Sun Mar 20 13:37:54 2016

Remove Dr. Memory suppressions and exclusions that are no longer needed.

Remove the suppressions and exclusions put in place to work around Dr. Memory bugs that are now fixed.

BUG= 594614 , 594618 , 594785 , 594618 , 594808 , 595158 , 595490 
TBR=reillyg,oshima
NOTRY=true

Review URL: https://codereview.chromium.org/1817853002

Cr-Commit-Position: refs/heads/master@{#382222}

[modify] https://crrev.com/899620623be8f7b5004cea13f7da96ff7ca80c27/tools/valgrind/drmemory/suppressions_full.txt
[modify] https://crrev.com/899620623be8f7b5004cea13f7da96ff7ca80c27/tools/valgrind/gtest_exclude/unit_tests.gtest-drmemory_win32.txt

Sign in to add a comment