google / sanitizers

AddressSanitizer, ThreadSanitizer, MemorySanitizer
Other
11.46k stars 1.03k forks source link

invalid memory access not detected with str.c_str()[1] when str is an empy std::string #1148

Open dpelle opened 5 years ago

dpelle commented 5 years ago

asan does not detect any bug in this buggy code which accesses beyond s.c_str() end of string (where s is an empty std::string).

$ cat bug.cpp
#include <string>
#include <stdio.h>
int main()
{
  const std::string s;
  if (s.c_str()[1] == '\0') // Bug, unnoticed by asan!?
  {
    fprintf(stderr, "1\n");
  }
}

$ g++-7 -g -fsanitize=address bug.cpp
$ ./a.out
1
$ clang++-9 -g -fsanitize=address bug.cpp
$ ./a.out
1

Bug is not found whether I build with the old or without the new c++ ABI (i.e. with -D_GLIBCXX_USE_CXX11_ABI=0 or -D_GLIBCXX_USE_CXX11_ABI=1)

That code may look silly but it's a simplification of a real bug that I found in some code.

I realize that, strictly speaking, it is probably not a bug in clang, but rather a missing instrumentation opportunity in libstdc++ (memory positioning) so I hope it's OK to report it here.

Tested on Ubuntu-18.04.

$ g++-7 -v
Using built-in specs.
COLLECT_GCC=g++-7
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/7/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 7.4.0-1ubuntu1~18.04.1' --with-bugurl=file:///usr/share/doc/gcc-7/README.Bugs --enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++ --prefix=/usr --with-gcc-major-version-only --program-suffix=-7 --program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-vtable-verify --enable-libmpx --enable-plugin --enable-default-pie --with-system-zlib --with-target-system-zlib --enable-objc-gc=auto --enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-offload-targets=nvptx-none --without-cuda-driver --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 7.4.0 (Ubuntu 7.4.0-1ubuntu1~18.04.1) 
$ clang++-9 -v
clang version 9.0.0-svn372167-1~exp1~20190917193124.56 (branches/release_90)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Found candidate GCC installation: /usr/bin/../lib/gcc/i686-linux-gnu/8
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/7
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/8
Found candidate GCC installation: /usr/lib/gcc/i686-linux-gnu/8
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/7
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/7.4.0
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/8
Selected GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0
Candidate multilib: .;@m64
Candidate multilib: 32;@m32
Candidate multilib: x32;@mx32
Selected multilib: .;@m64
eugenis commented 5 years ago

It's a short string optimization. You can see that ASan detects overflow starting with offset 23 with libc++ and offset 16 with libstdc++, and the report point to the stack allocation of std::string object itself, not to its heap-allocated buffer.

We could fix this by annotating std::string code with __annotate_contiguous_container (same as std::vector in libc++), but these are often more trouble than they are worth.

On Sun, Oct 6, 2019 at 1:09 AM Dominique Pellé notifications@github.com wrote:

asan does not detect any bug in this buggy code which accesses beyond s.c_str() end of string (where s is an empty std::string).

$ cat bug.cpp

include

include

int main() { const std::string s; if (s.c_str()[1] == '\0') // Bug, unnoticed by asan!? { fprintf(stderr, "1\n"); } }

$ g++-7 -g -fsanitize=address bug.cpp $ ./a.out 1 $ clang++-9 -g -fsanitize=address bug.cpp $ ./a.out 1

That code may look silly but it's a simplification of a real bug that I found in some code.

I realize that, strictly speaking, it is probably not a bug in clang, but rather a missing instrumentation opportunity in libstdc++ (memory positioning) so I hope it's OK to report it here.

Tested on Ubuntu-18.04.

$ g++-7 -v Using built-in specs. COLLECT_GCC=g++-7 COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/7/lto-wrapper OFFLOAD_TARGET_NAMES=nvptx-none OFFLOAD_TARGET_DEFAULT=1 Target: x86_64-linux-gnu Configured with: ../src/configure -v --with-pkgversion='Ubuntu 7.4.0-1ubuntu1~18.04.1' --with-bugurl=file:///usr/share/doc/gcc-7/README.Bugs --enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++ --prefix=/usr --with-gcc-major-version-only --program-suffix=-7 --program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-vtable-verify --enable-libmpx --enable-plugin --enable-default-pie --with-system-zlib --with-target-system-zlib --enable-objc-gc=auto --enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-offload-targets=nvptx-none --without-cuda-driver --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu Thread model: posix gcc version 7.4.0 (Ubuntu 7.4.0-1ubuntu1~18.04.1)

$ clang++-9 -v clang version 9.0.0-svn372167-1~exp1~20190917193124.56 (branches/release_90) Target: x86_64-pc-linux-gnu Thread model: posix InstalledDir: /usr/bin Found candidate GCC installation: /usr/bin/../lib/gcc/i686-linux-gnu/8 Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/7 Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0 Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/8 Found candidate GCC installation: /usr/lib/gcc/i686-linux-gnu/8 Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/7 Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/7.4.0 Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/8 Selected GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0 Candidate multilib: .;@m64 Candidate multilib: 32;@m32 Candidate multilib: x32;@mx32 Selected multilib: .;@m64

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/google/sanitizers/issues/1148?email_source=notifications&email_token=AADG4SQ6B3ZAMIQUOBUA2ELQNGMMVA5CNFSM4I527UIKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HP4GSWQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AADG4ST7OUS4MEZWTSVZILLQNGMMVANCNFSM4I527UIA .

dpelle commented 5 years ago

@eugenis write:

It's a short string optimization. You can see that ASan detects overflow starting with offset 23 with libc++ and offset 16 with libstdc++

The invalid memory is also not detected by the old c++ ABI (i.e. when building with -D_GLIBCXX_USE_CXX11_ABI=0) which does not use small string optimization.

We could fix this by annotating std::string code with __annotate_contiguous_container (same as std::vector in libc++), but these are often more trouble than they are worth.

I'm curious to know what kind of trouble annotating std::string could cause. Being able to detect more bugs with asan would seem useful.

eugenis commented 5 years ago

With -D_GLIBCXX_USE_CXX11_ABI=0, something special happens for empty strings. Initialize to "aa" and the overflow will be detected precisely.

Annotations are problematic when mixing instrumented and non-instrumented code. Since they are in the libstdc++/libc++ headers, the generated code is emitted in the user object files; a non-instrumented copy of ex. vector::push_back will not update ASan shadow for the new element; then an instrumented access with produce a false error report.

On Mon, Oct 7, 2019 at 1:49 PM Dominique Pellé notifications@github.com wrote:

@eugenis https://github.com/eugenis write:

It's a short string optimization. You can see that ASan detects overflow starting with offset 23 with libc++ and offset 16 with libstdc++

The invalid memory is also not detected by the old c++ ABI (i.e. when building with -D_GLIBCXX_USE_CXX11_ABI=0) which does not use small string optimization.

We could fix this by annotating std::string code with __annotate_contiguous_container (same as std::vector in libc++), but these are often more trouble than they are worth.

I'm curious to know what kind of trouble annotating std::string could cause. Being able to detect more bugs with asan would seem useful.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/sanitizers/issues/1148?email_source=notifications&email_token=AADG4SQLFEJ6RXHIFHXV2GLQNOOHPA5CNFSM4I527UIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEARYG5A#issuecomment-539198324, or mute the thread https://github.com/notifications/unsubscribe-auth/AADG4ST3SA3KW5ASHHOUZWLQNOOHPANCNFSM4I527UIA .

FruitClover commented 5 years ago

The invalid memory is also not detected by the old c++ ABI (i.e. when building with -D_GLIBCXX_USE_CXX11_ABI=0) which does not use small string optimization.

For empty COW strings these is common static storage (_S_empty_rep()) inside library, you could configure libstdc++ with --enable-fully-dynamic-string to detect this.