andmer / address-sanitizer

Automatically exported from code.google.com/p/address-sanitizer
0 stars 0 forks source link

failing to intercept strdup #148

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
Repro: http://trac.wxwidgets.org/ticket/15010
Users complain that they see a false report. 
The memory passed to asan's free is coming from strdup, 
which does not get intercepted (and uses libc's malloc which uses sbrk)

I'll try to make a small repro. Meanwhile, any ideas? 

Original issue reported on code.google.com by konstant...@gmail.com on 5 Feb 2013 at 9:34

GoogleCodeExporter commented 9 years ago
I tried to compile the wxwidgets code with pure gcc: 

cd wxWidgets/
mkdir build-gcc
cd build-gcc 
../configure
make -j30 
cd samples/minimal/
make
./minimal 

If I add my own implementation of strdup to samples/minimal/minimal.cpp it does 
not get called!

diff --git a/samples/minimal/minimal.cpp b/samples/minimal/minimal.cpp
index a78e462..d4a2deb 100644
--- a/samples/minimal/minimal.cpp
+++ b/samples/minimal/minimal.cpp
@@ -198,3 +198,13 @@ void MyFrame::OnAbout(wxCommandEvent& WXUNUSED(event))
                  wxOK | wxICON_INFORMATION,
                  this);
 }
+extern "C" {
+  char *strdup(const char *x) {
+    fprintf(stderr, "MyStrdup\n");
+    size_t len = strlen(x) + 1;
+    char *res = (char*)malloc(len);
+    memcpy(res, x, len);
+    return res;
+  }
+}

% nm ./minimal | grep strdup 
0000000000409f20 T strdup
% gdb -q ./minimal 
...
(gdb) b strdup
Breakpoint 1 at 0x409f20
(gdb) r
Starting program: /home/kcc/tmp/wxWidgets/build-gcc/samples/minimal/minimal 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/grte/v3/lib64/libthread_db.so.1".

Breakpoint 1, __GI___strdup (s=0x7ffff676eeea <_nl_C_name> "C") at strdup.c:41
41      strdup.c: No such file or directory.
(gdb) bt
#0  __GI___strdup (s=0x7ffff676eeea <_nl_C_name> "C") at strdup.c:41
#1  0x00007ffff71e98d3 in wxLocale::GetSystemEncodingName() () from 
/home/kcc/tmp/wxWidgets/build-gcc/lib/libwx_baseu-2.9.so.5
#2  0x00007ffff71e9aed in wxLocale::GetSystemEncoding() () from 
/home/kcc/tmp/wxWidgets/build-gcc/lib/libwx_baseu-2.9.so.5
#3  0x00007ffff72118a2 in wxCSConv::SetEncoding(wxFontEncoding) () from 
/home/kcc/tmp/wxWidgets/build-gcc/lib/libwx_baseu-2.9.so.5
#4  0x00007ffff721854b in wxCSConv::wxCSConv(wxFontEncoding) () from 
/home/kcc/tmp/wxWidgets/build-gcc/lib/libwx_baseu-2.9.so.5
#5  0x00007ffff7218841 in wxGet_wxConvLocalPtr() () from 
/home/kcc/tmp/wxWidgets/build-gcc/lib/libwx_baseu-2.9.so.5
#6  0x00007ffff7153c02 in _GLOBAL__sub_I_strconv.cpp () from 
/home/kcc/tmp/wxWidgets/build-gcc/lib/libwx_baseu-2.9.so.5
#7  0x00007ffff7de9306 in call_init (l=<optimized out>, argc=1, 
argv=0x7fffffffe288, env=0x7fffffffe298) at dl-init.c:85
#8  0x00007ffff7de93df in call_init (env=<optimized out>, argv=<optimized out>, 
argc=<optimized out>, l=<optimized out>) at dl-init.c:52
#9  _dl_init (main_map=0x7ffff7ffe2c8, argc=1, argv=0x7fffffffe288, 
env=0x7fffffffe298) at dl-init.c:134
#10 0x00007ffff7ddb6ea in _dl_start_user () from /lib64/ld-linux-x86-64.so.2
#11 0x0000000000000001 in ?? ()
#12 0x00007fffffffe548 in ?? ()
#13 0x0000000000000000 in ?? ()

Original comment by konstant...@gmail.com on 5 Feb 2013 at 10:14

GoogleCodeExporter commented 9 years ago
FTR, I think I had a similar problem with strdup on Windows.

Original comment by timurrrr@google.com on 5 Feb 2013 at 10:40

GoogleCodeExporter commented 9 years ago
Symbolic linking (wx is a shared library in this setup?)?
Some kind of preprocessor magic?

Original comment by euge...@google.com on 5 Feb 2013 at 10:46

GoogleCodeExporter commented 9 years ago
Running a pure-gcc build with custom strdup added to 
samples/minimal/minimal.cpp:
% nm ../../lib/libwx_baseu-2.9.so.5 | grep strdup
                 U strdup@@GLIBC_2.2.5
% nm  minimal | grep strdup
0000000000409f90 T strdup
% 

Disassm for libwx_baseu-2.9.so.5
Dump of assembler code for function _ZN8wxLocale21GetSystemEncodingNameEv:
   0x00007ffff71e9890 <+0>:     mov    %rbx,-0x20(%rsp)
   0x00007ffff71e9895 <+5>:     mov    %rbp,-0x18(%rsp)
   0x00007ffff71e989a <+10>:    xor    %esi,%esi
   0x00007ffff71e989c <+12>:    mov    %r12,-0x10(%rsp)
   0x00007ffff71e98a1 <+17>:    mov    %r13,-0x8(%rsp)
   0x00007ffff71e98a6 <+22>:    sub    $0x68,%rsp
   0x00007ffff71e98aa <+26>:    mov    0x384eff(%rip),%r12        # 0x7ffff756e7b0
   0x00007ffff71e98b1 <+33>:    movq   $0x0,0x8(%rdi)
   0x00007ffff71e98b9 <+41>:    mov    %rdi,%rbx
   0x00007ffff71e98bc <+44>:    lea    0x18(%r12),%rax
   0x00007ffff71e98c1 <+49>:    mov    %rax,(%rdi)
   0x00007ffff71e98c4 <+52>:    xor    %edi,%edi
   0x00007ffff71e98c6 <+54>:    callq  0x7ffff714eda0 <setlocale@plt>
   0x00007ffff71e98cb <+59>:    mov    %rax,%rdi
   0x00007ffff71e98ce <+62>:    callq  0x7ffff7151e90 <strdup@plt>
=> 0x00007ffff71e98d3 <+67>:    mov    %rax,%rbp
   0x00007ffff71e98d6 <+70>:    mov    %rax,%rcx

And this call ends up calling libc's strdup. 

Original comment by konstant...@gmail.com on 5 Feb 2013 at 11:42

GoogleCodeExporter commented 9 years ago
Ok, mystery solved. 

% head strdup?.cc version-script 
==> strdup1.cc <==
#include <stdio.h>
extern char *z;
int main(int argc, char **argv) {
  printf("z: %p %s\n", z, z);
  return 0;
}

==> strdup2.cc <==
#include <string.h>
char *z = strdup("FFFFF");

==> version-script <==
WXU_2.9 {
        *;
};

% clang++ -fsanitize=address -O3  -fPIC strdup2.cc -shared -o strdup2.so; 
clang++ -fsanitize=address strdup2.so ~/tmp/strdup1.cc -Wl,-rpath=.   
-Wl,--version-script,./version-script   && ./a.out 
z: 0x451f040 FFFFF  <<<<<<<<<<<<<<<< BAD

% clang++ -fsanitize=address -O3  -fPIC strdup2.cc -shared -o strdup2.so; 
clang++ -fsanitize=address strdup2.so ~/tmp/strdup1.cc -Wl,-rpath=.  && ./a.out 
z: 0x60080000dff0 FFFFF  <<<<<<<<<<<<<<<<<< GOOD

the version script used in the wxWidgets is unfriendly to asan. 
Dunno why (any idea?)
Anyway, I don't consider this an asan bug. 

Original comment by konstant...@gmail.com on 5 Feb 2013 at 12:12

GoogleCodeExporter commented 9 years ago
Someone needs to read up about versioned symbol lookup, or ask khim.

Original comment by euge...@google.com on 5 Feb 2013 at 12:21

GoogleCodeExporter commented 9 years ago
Anyway, this is still a bug. Is it possible to at least detect this situation 
and warn the user? 

Original comment by euge...@google.com on 5 Feb 2013 at 12:24

GoogleCodeExporter commented 9 years ago
How? 

Original comment by konstant...@gmail.com on 5 Feb 2013 at 12:25

GoogleCodeExporter commented 9 years ago
As Victor is too lazy to answer here:
we could use asm(".symver") to pin our interceptors to empty ("") version. This 
stuff is stronger than version scripts!

Original comment by euge...@google.com on 5 Feb 2013 at 3:09

GoogleCodeExporter commented 9 years ago
Any update on this?

Original comment by ettl.mar...@gmail.com on 21 Feb 2013 at 3:19

GoogleCodeExporter commented 9 years ago
No. While we may fix this in future, I still suggest to not use the version 
script with asan. 

Original comment by konstant...@gmail.com on 22 Feb 2013 at 9:13

GoogleCodeExporter commented 9 years ago
Can someone please explain what's going on in this bug? I'm hitting this issue 
in Chrome when third-party libraries leak the result of strdup:

Indirect leak of 89 byte(s) in 1 object(s) allocated from:
    #0 0x4a3893 in __interceptor_realloc /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:95
    #1 0x7f1531fda949 in __GI___vasprintf_chk /build/buildd/eglibc-2.15/debug/vasprintf_chk.c:90

Indirect leak of 49 byte(s) in 1 object(s) allocated from:
    #0 0x4a3699 in __interceptor_malloc /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x7f1531f58d81 in __GI___strdup /build/buildd/eglibc-2.15/string/strdup.c:43

...

I would like to have interceptors for strdup and vasprintf which would avoid 
trying to unwind stack through libc. From this discussion I'm getting the 
impression that that is not trivial, but I'm confused as to why.

Original comment by earth...@google.com on 28 Nov 2013 at 4:55

GoogleCodeExporter commented 9 years ago
FYI: ASan intercepts strdup to make sure we have properly unwinded stack trace,
but not __GI___strdup (I don't know why is it called). IIRC we don't intercept 
printf and friends.

Original comment by samso...@google.com on 29 Nov 2013 at 8:09

GoogleCodeExporter commented 9 years ago
I think we are discussing multiple issues here.
  1. The original bug is about version scripts messing up sanitizer interceptors.
  2. comment #12, based on _chk ending, is due to _FORTIFY_SOURCE. It may be fixed by intercepting _chk variants, or adding diagnostics, or maybe even undefining _FORTIFY_SOURCE in the driver.
  3. __GI___strdup is a non-dynamic symbol with the same address as strdup. We don't need to intercept it, but we may want to tweak the symbolizer to prefer dynamic symbols.

Original comment by euge...@google.com on 29 Nov 2013 at 9:49

GoogleCodeExporter commented 9 years ago
I don't your comment in (3), this is not related to symbolizer. Apparently, the 
program calls __GI__strdup() from libc instead of strdup() from ASan runtime. 
How can we avoid this?

Original comment by samso...@google.com on 29 Nov 2013 at 9:52

GoogleCodeExporter commented 9 years ago
OK. __GI__strdup is still a symbolizer deficiency, but it is not clear why it 
got called without going through an interceptor.
My guess is something else in libc called strdup internally. Please re-run the 
test with slow unwind in malloc.

Original comment by euge...@google.com on 29 Nov 2013 at 10:08

GoogleCodeExporter commented 9 years ago
Folks, please stop hijacking a bug. 

Original comment by konstant...@gmail.com on 29 Nov 2013 at 10:10

GoogleCodeExporter commented 9 years ago
Version scripts can also be used to make certain symbols local. I guess 
asm(".symver") workaround won't work in this case.

Ex.: 
https://android.googlesource.com/platform/art/+/android-l-preview_r2/sigchainlib
/version-script.txt

Original comment by euge...@google.com on 11 Dec 2014 at 10:15

GoogleCodeExporter commented 9 years ago
I don't think people are hijacking the bug - I think attributing this issue 
version scripts was inaccurate to begin with. The root cause seems to be ASAN 
simply not properly intercepting strdup under any conditions. I'm noticing the 
same symptoms on my system and we're not using version scripts. Additionally, 
the call stack for strdup looks identical to the examples presented so far - 
__GI___strdup is called - even without the use of version scripts.

IMHO, this is a true ASAN bug and it's affecting a lot of code using gcc 4.8.0+ 
now because the GCC team has made it available through the -fsanitize=address 
flag. Of course, we can always not use that flag, but is that what you guys 
really want?

Original comment by john.cal...@gmail.com on 8 Jul 2015 at 7:26

GoogleCodeExporter commented 9 years ago
re: #19
Are you using _FORTIFY_SOURCE? Could you provide a small test case, or at least 
a stack trace that show where this __GI__strdup is called from?

See #14. This bug is not only about version scripts, but we have not seen any 
actionable report of the non-version-script case.

Original comment by euge...@google.com on 8 Jul 2015 at 8:03

GoogleCodeExporter commented 9 years ago
>> I don't think people are hijacking the bug
That's not the most interesting topic to argue about. 

>> The root cause seems to be ASAN simply not properly intercepting strdup 
under any conditions.

False. 

% cat strdup-oob.cc 
#include <string.h>
int main() {
  return strdup("abc")[5];
}
% clang -g -fsanitize=address strdup-oob.cc && 
ASAN_OPTIONS=strip_path_prefix=$HOME/ ./a.out 
=================================================================
==27103==ERROR: AddressSanitizer: heap-buffer-overflow on address 
0x60200000eff5 at pc 0x0000004cde1d bp 0x7ffef4c88c90 sp 0x7ffef4c88c88
READ of size 1 at 0x60200000eff5 thread T0
    #0 0x4cde1c in main tmp/strdup-oob.cc:3:10
    #1 0x7f19fe8e4ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
    #2 0x4178a5 in _start (tmp/a.out+0x4178a5)

0x60200000eff5 is located 1 bytes to the right of 4-byte region 
[0x60200000eff0,0x60200000eff4)
allocated by thread T0 here:
    #0 0x493543 in strdup llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:590:3
    #1 0x4cdddd in main tmp/strdup-oob.cc:3:10
    #2 0x7f19fe8e4ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287

As you can see, strdup is properly intercepted when using the clang version of 
AddressSanitizer. 

With gcc (4.8.4-2ubuntu1~14.04), it does not happen:

% gcc -g -fsanitize=address strdup-oob.cc && 
ASAN_OPTIONS=strip_path_prefix=$HOME/ ./a.out 2>&1 | 
~/llvm/projects/compiler-rt/lib/asan/scripts/asan_symbolize.py 
=================================================================
==27162== ERROR: AddressSanitizer: heap-buffer-overflow on address 
0x60040000dff5 at pc 0x400843 bp 0x7ffecc10f940 sp 0x7ffecc10f938
READ of size 1 at 0x60040000dff5 thread T0
LLVMSymbolizer: error reading file: No such file or directory.
addr2line: 'tmp/a.out': No such file
    #0 0x400842 in
    #1 0x7feda6c47ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287:0
    #2 0x400738 in
0x60040000dff5 is located 1 bytes to the right of 4-byte region 
[0x60040000dff0,0x60040000dff4)
allocated by thread T0 here:
    #0 0x7feda700046a in malloc _asan_rtl_:0
    #1 0x7feda6cae839 in __strdup /build/buildd/eglibc-2.19/string/strdup.c:42:0

But with the more recent gcc (2-3 months old trunk) it works again:
allocated by thread T0 here:
    #0 0x41f65f in __interceptor_strdup ../../../../gcc/libsanitizer/asan/asan_interceptors.cc:514
    #1 0x4a8a2b in main tmp/strdup-oob.cc:3

So, please be more specific about your platform, your compiler version and the 
exact problem you experience. 

Original comment by konstant...@gmail.com on 8 Jul 2015 at 8:12

GoogleCodeExporter commented 9 years ago

Original comment by ramosian.glider@gmail.com on 30 Jul 2015 at 9:05

GoogleCodeExporter commented 9 years ago
Adding Project:AddressSanitizer as part of GitHub migration.

Original comment by ramosian.glider@gmail.com on 30 Jul 2015 at 9:06