My favorites | Sign in
Project Home Downloads Wiki Issues Code Search
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 178677: Trunk ASan reports global-buffer-overflow errors in sqlite
6 people starred this issue and may be notified of changes. Back to list
 
Project Member Reported by gli...@chromium.org, Feb 27, 2013
From unit_tests (see the log at our internal bot: http://racer-z600.msk.corp.google.com:8010/builders/linux-chrome-asan/builds/52/steps/running%20unit_tests/logs/stdio):

[ RUN      ] WebDatabaseMigrationTest.MigrateEmptyToCurrent
=================================================================
==25254== ERROR: AddressSanitizer: global-buffer-overflow on address 0x000011d3dde5 at pc 0x5ab21a bp 0x7fff00659450 sp 0x7fff00659428
READ of size 10 at 0x000011d3dde5 thread T0
    #0 0x5ab219 in __interceptor_memcmp _asan_rtl_
    #1 0xa1edc08 in fillInUnixFile /usr/local/google/chrome-asan/src/out/Release/../../third_party/sqlite/amalgamation/sqlite3.c:28654
    #2 0xa1efe7c in unixOpen /usr/local/google/chrome-asan/src/out/Release/../../third_party/sqlite/amalgamation/sqlite3.c:29294
    #3 0xa238fda in sqlite3OsOpen /usr/local/google/chrome-asan/src/out/Release/../../third_party/sqlite/amalgamation/sqlite3.c:14087
    #4 0xa2284e7 in openDatabase /usr/local/google/chrome-asan/src/out/Release/../../third_party/sqlite/amalgamation/sqlite3.c:109332
    #5 0xce46517 in sql::Connection::OpenInternal(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /usr/local/google/chrome-asan/src/out/Release/../../sql/connection.cc:604
    #6 0xce46348 in sql::Connection::Open(base::FilePath const&) /usr/local/google/chrome-asan/src/out/Release/../../sql/connection.cc:134
    #7 0x61d94c9 in WebDatabase::Init(base::FilePath const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /usr/local/google/chrome-asan/src/out/Release/../../chrome/browser/webdata/web_database.cc:110
    #8 0x42e12bb in WebDatabaseMigrationTest_MigrateEmptyToCurrent_Test::TestBody() /usr/local/google/chrome-asan/src/out/Release/../../chrome/browser/webdata/web_database_migration_unittest.cc:234
    #9 0x8502e27 in testing::Test::Run() /usr/local/google/chrome-asan/src/out/Release/../../testing/gtest/src/gtest.cc:2067
    #10 0x8504c84 in testing::TestInfo::Run() /usr/local/google/chrome-asan/src/out/Release/../../testing/gtest/src/gtest.cc:2244
    #11 0x8505b5c in testing::TestCase::Run() /usr/local/google/chrome-asan/src/out/Release/../../testing/gtest/src/gtest.cc:2351
    #12 0x85131a0 in testing::internal::UnitTestImpl::RunAllTests() /usr/local/google/chrome-asan/src/out/Release/../../testing/gtest/src/gtest.cc:4177
    #13 0x851273d in testing::UnitTest::impl() /usr/local/google/chrome-asan/src/out/Release/../../testing/gtest/src/gtest.cc:2051
    #14 0xd7fb6ea in base::TestSuite::Run() /usr/local/google/chrome-asan/src/out/Release/../../base/test/test_suite.cc:159
    #15 0xcddeaf1 in main /usr/local/google/chrome-asan/src/out/Release/../../chrome/test/base/run_all_unittests.cc:11
    #16 0x7f25f75c576c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226
    #17 0x5ba67c in _start ??:0
0x000011d3dde5 is located 59 bytes to the left of global variable '.str4 (../../third_party/sqlite/amalgamation/sqlite3.c)' (0x11d3de20) of size 10
  '.str4 (../../third_party/sqlite/amalgamation/sqlite3.c)' is ascii string 'unix-none'
0x000011d3dde5 is located 0 bytes to the right of global variable '.str3 (../../third_party/sqlite/amalgamation/sqlite3.c)' (0x11d3dde0) of size 5
  '.str3 (../../third_party/sqlite/amalgamation/sqlite3.c)' is ascii string 'unix'
Shadow bytes around the buggy address:
  0x00008239fb60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00008239fb70: 00 00 00 00 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9
  0x00008239fb80: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
  0x00008239fb90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00008239fba0: 00 00 00 00 f9 f9 f9 f9 f9 f9 f9 f9 00 02 f9 f9
=>0x00008239fbb0: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9[05]f9 f9 f9
  0x00008239fbc0: f9 f9 f9 f9 00 02 f9 f9 f9 f9 f9 f9 00 05 f9 f9
  0x00008239fbd0: f9 f9 f9 f9 00 00 00 00 00 f9 f9 f9 f9 f9 f9 f9
  0x00008239fbe0: 00 00 07 f9 f9 f9 f9 f9 00 00 00 00 00 00 f9 f9
  0x00008239fbf0: f9 f9 f9 f9 00 00 00 06 f9 f9 f9 f9 00 00 05 f9
  0x00008239fc00: f9 f9 f9 f9 00 00 05 f9 f9 f9 f9 f9 00 04 f9 f9
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:     fa
  Heap righ redzone:     fb
  Freed Heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe
==25254== ABORTING

This shouldn't be a security critical error, but it's better to be fixed before the next Clang roll.

To reproduce the problem, change tools/clang/scripts/update.sh to have CLANG_REVISION=176174 and build unit_tests with ASan GYP defines ('asan=1 linux_use_tcmalloc=0 release_extra_cflags="-g" ')
Feb 27, 2013
#1 gli...@chromium.org
(No comment was entered for this change.)
Cc: kcc@chromium.org
Feb 27, 2013
#2 gli...@chromium.org
CCing Abhishek. This error also occurs at Chrome startup, so it must be fixed before the next Clang roll, otherwise it'll break ClusterFuzz.
Cc: infe...@chromium.org
Labels: -Pri-2 Pri-1
Feb 28, 2013
#3 gli...@chromium.org
The following diff fixes the problem with unit_tests:
--------------------------------------------------------------
$ git diff third_party/sqlite/amalgamation/sqlite3.c
diff --git a/third_party/sqlite/amalgamation/sqlite3.c b/third_party/sqlite/amalgamation/sqlite3.c
index b9088a5..b186489 100644
--- a/third_party/sqlite/amalgamation/sqlite3.c
+++ b/third_party/sqlite/amalgamation/sqlite3.c
@@ -28651,7 +28651,7 @@ int fillInUnixFile(
   OSTRACE(("OPEN    %-3d %s\n", h, zFilename));
   pNew->h = h;
   pNew->zPath = zFilename;
-  if( memcmp(pVfs->zName,"unix-excl",10)==0 ){
+  if( strncmp(pVfs->zName,"unix-excl",10)==0 ){
     pNew->ctrlFlags = UNIXFILE_EXCL;
   }else{
     pNew->ctrlFlags = 0;
--------------------------------------------------------------

Scott, is it ok to land this to Chrome or should it be upstreamed?

According to Joseph Myers (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55309#c55):
================
I believe the arguments to memcmp must point to objects with at least the 
given number of bytes.  (For strcmp, they must point to NUL-terminated 
strings.  For strncmp, they must point to objects that either have at 
least the given number of bytes or have bytes present up to a NUL within 
that number of bytes - there's no guarantee that comparison stops early 
when characters differ except for not reading after a NUL.  By comparison, 
the array passed to memchr may be shorter than the given length if a 
matching character is found early - see the wording added in C11 for 
memchr for alignment with POSIX.  But memcmp has no such special rule.)
================
Feb 28, 2013
#4 thakis@chromium.org
If you pass the length of the known string to strncmp(), it's not any better than just using strcmp(), is it? Passing a max length of zName would make more sense I think.

I think this should be upstreamed.
Feb 28, 2013
#5 gli...@chromium.org
> If you pass the length of the known string to strncmp(), it's not any better than just using strcmp(), is it?

True.

$ git diff third_party/sqlite/amalgamation/sqlite3.c
diff --git a/third_party/sqlite/amalgamation/sqlite3.c b/third_party/sqlite/amalgamation/sqlite3.c
index b9088a5..8079c83 100644
--- a/third_party/sqlite/amalgamation/sqlite3.c
+++ b/third_party/sqlite/amalgamation/sqlite3.c
@@ -28651,7 +28651,7 @@ int fillInUnixFile(
   OSTRACE(("OPEN    %-3d %s\n", h, zFilename));
   pNew->h = h;
   pNew->zPath = zFilename;
-  if( memcmp(pVfs->zName,"unix-excl",10)==0 ){
+  if( strcmp(pVfs->zName,"unix-excl")==0 ){
     pNew->ctrlFlags = UNIXFILE_EXCL;
   }else{
     pNew->ctrlFlags = 0;


Mar 2, 2013
#6 infe...@chromium.org
Thanks a lot Alex for ccing me, lets make sure we get the fix in before the next roll. Pwnium is next week, and ClusterFuzz will break really badly if we have a startup bug like this.
Mar 3, 2013
#7 infe...@chromium.org
Just a fyi, Firefox on Clang trunk needed these 3. This file uses a whole ton of memcmp's, but lets try to fix these 3 for chromium as well. If we can do the clang roll post pwnium, that would be great.

diff -r e9d4487af28d db/sqlite3/src/sqlite3.c
--- a/db/sqlite3/src/sqlite3.c	Sat Mar 02 03:18:27 2013 -0800
+++ b/db/sqlite3/src/sqlite3.c	Sun Mar 03 08:48:36 2013 -0800
@@ -27637,7 +27637,7 @@
                            "psow", SQLITE_POWERSAFE_OVERWRITE) ){
     pNew->ctrlFlags |= UNIXFILE_PSOW;
   }
-  if( memcmp(pVfs->zName,"unix-excl",10)==0 ){
+  if( strncmp(pVfs->zName,"unix-excl",10)==0 ){
     pNew->ctrlFlags |= UNIXFILE_EXCL;
   }
 
@@ -63009,7 +63009,7 @@
   if( zName ){
     for(i=0; i<p->nzVar; i++){
       const char *z = p->azVar[i];
-      if( z && memcmp(z,zName,nName)==0 && z[nName]==0 ){
+      if( z && strncmp(z,zName,nName)==0 && z[nName]==0 ){
         return i+1;
       }
     }
@@ -74405,7 +74405,7 @@
       */
       ynVar i;
       for(i=0; i<pParse->nzVar; i++){
-        if( pParse->azVar[i] && memcmp(pParse->azVar[i],z,n+1)==0 ){
+        if( pParse->azVar[i] && strncmp(pParse->azVar[i],z,n+1)==0 ){
           pExpr->iColumn = x = (ynVar)i+1;
           break;
         }
Mar 3, 2013
#8 infe...@chromium.org
(No comment was entered for this change.)
Cc: ifrat...@google.com
Mar 3, 2013
#9 shess@chromium.org
Any reason not to use straight strcmp() for all three?  Removing the z[nName] check for the middle one.
Mar 3, 2013
#10 infe...@chromium.org
No, we should be able to use strcmp for all of these. Also, there are 3-4 instances that should be fixed. Just search regex memcmp.*name.
Cc: decoder...@googlemail.com
Mar 4, 2013
#11 thakis@chromium.org
(No comment was entered for this change.)
Blocking: chromium:177235
Mar 4, 2013
#12 bugdro...@chromium.org
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=185923

------------------------------------------------------------------------
r185923 | glider@chromium.org | 2013-03-04T17:59:20.398064Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/tools/build/scripts/slave/runtest.py?r1=185923&r2=185922&pathrev=185923

Disable the aggressive memcmp checks under ASan.
There currently are several bugs that prevent Chrome from starting up.

BUG=178677,178404

Review URL: https://chromiumcodereview.appspot.com/12398012
------------------------------------------------------------------------
Mar 4, 2013
#13 phajdan.jr@chromium.org
Does ClusterFuzz run the above script (runtest.py)? If not, can we also apply the same workaround for ClusterFuzz? This will help migrate chromium.memory waterfall on time.
Cc: phajdan.jr@chromium.org
Mar 4, 2013
#14 infe...@chromium.org
ClusterFuzz does not use runtest.py, but i added the workaround strict_memcmp=0
Cc: -decoder...@googlemail.com
Mar 4, 2013
#15 infe...@chromium.org
(No comment was entered for this change.)
Cc: decoder...@googlemail.com
Mar 9, 2013
#16 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Area-Internals Cr-Internals
Mar 10, 2013
#17 gli...@chromium.org
This isn't a security bug, so I think it's fine if others can see it.
decoder.oh@googlemail.com, sorry, you're being un-CCed, because code.google.com doesn't allow me to keep you in the CC list. Please star the issue yourself.
Cc: -decoder...@googlemail.com
Labels: -Restrict-View-Google Performance-Memory-AddressSanitizer
Mar 19, 2013
#18 bugdro...@chromium.org
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=188977

------------------------------------------------------------------------
r188977 | glider@chromium.org | 2013-03-19T10:18:29.202034Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/app/chrome_exe_main_gtk.cc?r1=188977&r2=188976&pathrev=188977

Suppress ASan reports in memcmp in the official google-chrome-asan build.

BUG=178677,178404

Review URL: https://chromiumcodereview.appspot.com/12780011
------------------------------------------------------------------------
Apr 1, 2013
#19 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Performance-Memory-AddressSanitizer Stability-Memory-AddressSanitizer
May 3, 2013
#20 wfh@chromium.org
I feel dirty disabling strict memcmp checks in ASAN - did we decide whether we were going to upsteam these fixes?
May 3, 2013
#21 shess@chromium.org
My posting to sqlite-dev didn't generate any response, but ... when I look at the most recent SQLite code, two of the memcmp() cases I mentioned in the email are now strcmp().  I'll go see if I can figure out when what landed where.
May 8, 2013
#22 shess@chromium.org
http://www.sqlite.org/src/info/d73435587b is their fix, I'll backport.
May 9, 2013
#23 bugdro...@chromium.org
------------------------------------------------------------------------
r199345 | shess@chromium.org | 2013-05-10T02:52:43.862019Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/third_party/sqlite/src/src/os_unix.c?r1=199345&r2=199344&pathrev=199345
   M http://src.chromium.org/viewvc/chrome/trunk/src/third_party/sqlite/src/src/vdbeapi.c?r1=199345&r2=199344&pathrev=199345
   A http://src.chromium.org/viewvc/chrome/trunk/src/third_party/sqlite/memcmp.patch?r1=199345&r2=199344&pathrev=199345
   M http://src.chromium.org/viewvc/chrome/trunk/src/third_party/sqlite/src/src/expr.c?r1=199345&r2=199344&pathrev=199345
   M http://src.chromium.org/viewvc/chrome/trunk/src/third_party/sqlite/src/src/build.c?r1=199345&r2=199344&pathrev=199345
   M http://src.chromium.org/viewvc/chrome/trunk/src/third_party/sqlite/README.chromium?r1=199345&r2=199344&pathrev=199345
   M http://src.chromium.org/viewvc/chrome/trunk/src/third_party/sqlite/amalgamation/sqlite3.c?r1=199345&r2=199344&pathrev=199345
   M http://src.chromium.org/viewvc/chrome/trunk/src/third_party/sqlite/src/src/analyze.c?r1=199345&r2=199344&pathrev=199345

Backport SQLite memcmp patch.

http://www.sqlite.org/src/info/d73435587b

Verified that the amalgamation came out with all the right patches by
comparing the amalgamation diff to the appropriately-ordered diffs of
the original files.

BUG=178677

Review URL: https://chromiumcodereview.appspot.com/15070002
------------------------------------------------------------------------
May 10, 2013
#24 shess@chromium.org
OK, this fixed the #3/#5 item, and I believe that all three items in #7 are addressed (the last one is different in our version of SQLite, but I believe the expr.c patch is it for our version).  It also hit a few other cases not described here.

Quickly skimming the code, I see the following possibly-questionable cases still in sqlite3.c:
 - vxworksFindFileId(), obviously won't apply to us.
 - pager_open_journal() TimeMachine addition we made, I believe it's safe, but I'll circle back and adjust it.
 - replaceFunc, trimFunc, binCollFunc, I think these can receive blobs, so strcmp wouldn't be appropriate.
 - various places in fts3.

For those last two cases, they appear to be doing the right thing, such as using the minimum length of the inputs, or checking that one input is >= than the other, etc.  I'm pretty sure they aren't using it as a strcmp() short-cut of any sort, so they most likely had more thought about these issues.

I'm pretty sure the TimeMachine thing only could happen if we started using a custom vfs of some sort, so shouldn't cause any issues at this time.
Owner: gli...@chromium.org
Cc: shess@chromium.org
May 13, 2013
#25 bugdro...@chromium.org
------------------------------------------------------------------------
r199794 | shess@chromium.org | 2013-05-13T19:30:28.403233Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/third_party/sqlite/src/src/pager.c?r1=199794&r2=199793&pathrev=199794
   M http://src.chromium.org/viewvc/chrome/trunk/src/third_party/sqlite/amalgamation/sqlite3.c?r1=199794&r2=199793&pathrev=199794
   M http://src.chromium.org/viewvc/chrome/trunk/src/third_party/sqlite/mac_time_machine.patch?r1=199794&r2=199793&pathrev=199794

Convert degenerate memcmp() to strncmp() in SQLite TimeMachine patch.

BUG=178677

Review URL: https://chromiumcodereview.appspot.com/14651031
------------------------------------------------------------------------
Aug 8, 2013
#26 infe...@chromium.org
 Issue 270333  has been merged into this issue.
Sign in to add a comment

Powered by Google Project Hosting