dr asan inlined strlen false positive in libfontconfig.so

[GoogleCodeExporter]

This is running DRT on gPrecise.

The asan report has a crummy stack trace still:
=================================================================
==16664== ERROR: AddressSanitizer heap-buffer-overflow on address 
0x7fffef2c1f94 at pc 0x7ffff58620d3 bp 0x7fffffffa910 sp 0x7fffffffa908
READ of size 4 at 0x7fffef2c1f94 thread T0
    #0 0x7ffff58620d3 (/usr/lib/x86_64-linux-gnu/libfontconfig.so.1.4.4+0x80d3)
0x7fffef2c1f94 is located 0 bytes to the right of 22-byte region 
[0x7fffef2c1f80,0x7fffef2c1f96)
allocated by thread T0 here:
    #0 0x5463022 (/usr/local/google/home/rnk/chromium/src/out_asan/Release/DumpRenderTree+0x5463022)
    #1 0x7ffff586202d (/usr/lib/x86_64-linux-gnu/libfontconfig.so.1.4.4+0x802d)

I should fix that.

I got one from gdb:

#0  0x0000000005464964 in __asan_report_load4 ()
#1  0x00007ffff58620d3 in FcConfigFileExists (dir=0x7fffef2c1f80 "/etc/fonts", 
file=0x7ffff5879ab5 "fonts.conf") at fccfg.c:1671
#2  0x00007ffff5864465 in IA__FcConfigFilename (url=0x7ffff5879ab5 
"fonts.conf") at fccfg.c:1828
#3  0x00007ffff5877a16 in IA__FcConfigParseAndLoad (config=0x7fffefdf6480, 
name=0x0, complain=1) at fcxml.c:2459
#4  0x00007ffff586d177 in IA__FcInitLoadConfig () at fcinit.c:67
#5  0x00007ffff586d266 in IA__FcInitLoadConfigAndFonts () at fcinit.c:101
#6  0x00007ffff586d485 in IA__FcInit () at fcinit.c:124
#7  0x00000000004cf699 in setupFontconfig () at 
../../third_party/WebKit/Tools/DumpRenderTree/chromium/TestShellX11.cpp:86
#8  platformInit (argc=0x7fffef2c2094, argv=0x7fffef2c2094) at 
../../third_party/WebKit/Tools/DumpRenderTree/chromium/TestShellX11.cpp:189
#9  0x000000000045fc0d in main (argc=<optimized out>, argv=<optimized out>) at 
../../third_party/WebKit/Tools/DumpRenderTree/chromium/DumpRenderTree.cpp:123

Here's the disassembly which makes me think it's strlen.  I haven't verified 
that the code actually works or corresponds.

   0x00007ffff58620cb <+203>:   callq  0x7ffff585fda0 <strcat@plt>
---Type <return> to continue, or q <return> to quit---
   0x00007ffff58620d0 <+208>:   mov    %rbx,%rsi
=> 0x00007ffff58620d3 <+211>:   mov    (%rsi),%edx
   0x00007ffff58620d5 <+213>:   add    $0x4,%rsi
   0x00007ffff58620d9 <+217>:   lea    -0x1010101(%rdx),%eax
   0x00007ffff58620df <+223>:   not    %edx
   0x00007ffff58620e1 <+225>:   and    %edx,%eax
   0x00007ffff58620e3 <+227>:   and    $0x80808080,%eax
   0x00007ffff58620e8 <+232>:   je     0x7ffff58620d3 <FcConfigFileExists+211>

Maybe we can suppress this by looking for "and 0x8080808080" in the current bb? 
 Sounds pretty hacky.  We have a bunch of pattern matchers for this kind of 
thing in drmemory.

Original issue reported on code.google.com by rnk@google.com on 2 Aug 2012 at 8:05

[GoogleCodeExporter]

Do you have a Valgrind-like notion of suppressions? I think matching the stack 
just before reporting the error is the right thing to do.

Original comment by ramosian.glider@gmail.com on 3 Aug 2012 at 8:26

[GoogleCodeExporter]

+1 to glider@,
I think we should add general suppression support to DRASan (or DR itself?) 
rather than doing something ad-hoc.
Also, I'm afraid we'll have to replace str*/mem* functions similiar to what 
Valgrind and Dr.Memory and TSan do. Yes, this sucks :(

Original comment by timurrrr@google.com on 3 Aug 2012 at 8:51

[GoogleCodeExporter]

str*/mem* are already replaced by asan.  This is inlined, so we can't really 
replace it.  :(

I don't want to add suppression support because it will be super unreliable, 
but I think we have to eventually.  This library is built without 
-fno-omit-frame-pointer, so I will need to implement a shadow callstack in 
order to have reliable callstack walking.  The user will user have to install 
symbols, or we have to rely on the nearest guy in .dynsym and have weird 
symbols that change from system to system.

This is all crappy, but, I think unavoidable.

Shadow callstacks are also useful to drmemory, so this is probably worth doing 
as a DR extension so we can reuse it.  Currently in drmemory we have a whole 
pile of interesting heuristics, but we keep having problems with finnicky 
Windows system libraries.

Original comment by rnk@google.com on 3 Aug 2012 at 2:28

[GoogleCodeExporter]

Oh, this really sucks...

> str*/mem* are already replaced by asan.
Yeah, I always forget that on Linux you have the same strlen for all the 
libraries in the process rather than a copy for each library.

> This is inlined, so we can't really replace it.  :(
> This library is built without -fno-omit-frame-pointer
Is it the same problem like you've observed under Valgrind? Or was it Lei?..

Is there any way to use a different build of the library? Probably we 
can/should/must just rebuild the library with "clang -faddress-sanitizer" ?

Original comment by timurrrr@google.com on 3 Aug 2012 at 4:38

[GoogleCodeExporter]

That stack trace looks very familiar. It happens in memcheck as well.

Original comment by thes...@google.com on 3 Aug 2012 at 4:43

[GoogleCodeExporter]

Timur: one cannot simply rebuild every library with ASan, this is the reason 
for which we need the dynamic instrumentation.

Reid: how did you get that gdb stack? Did you install additional symbols or was 
the .eh_frame section enough to unwind the stack (I think the latter should be 
true)?

Original comment by ramosian.glider@gmail.com on 3 Aug 2012 at 4:46

[GoogleCodeExporter]

glider: yes, but rebuilding just fontconfig is much easier than re-building 
everything including closed-source libraries

Original comment by timurrrr@google.com on 3 Aug 2012 at 4:50

[GoogleCodeExporter]

Yeah, if you search, you can find similar examples of this coming up as an 
uninit read around the web.

I got the stack trace with gdb by installing symbols.  I assume it used 
.eh_frame/.debug_unwind to unwind.

I'm more inclined to implement a shadow callstack than to parse .eh_frame 
because the shadow callstack is portable to Windows.

For now I just added libfontconfig.so to the list of libs for which we don't 
instrument reads.  Overreads are less serious anyway.

Original comment by rnk@google.com on 3 Aug 2012 at 5:23

[GoogleCodeExporter]

> I'm more inclined to implement a shadow callstack
Only for the dynamic libs or for the main binary too?

Original comment by timurrrr@google.com on 3 Aug 2012 at 6:04

[GoogleCodeExporter]

Hm, I hadn't thought hard enough about it.  I think we can probably still get 
away without instrumenting the main binary.  When we want to turn the shadow 
stack into a full callstack, every time the retaddr on the stack doesn't match 
the next shadow frame, we can fall back onto a frame pointer walk for a few 
frames until it synchs up.

The hard parts with the shadow callstack are how do you get back on track after 
a longjmp or exception, or even more crazy, stack switching and user space 
threads.  Probably can't do that without annotations.

Original comment by rnk@google.com on 3 Aug 2012 at 7:24

[GoogleCodeExporter]

I thought maintaining the shadow callstack adds some extra slowdown which might 
be large enough compared to the usual ASan slowdown?

Original comment by timurrrr@google.com on 4 Aug 2012 at 8:06

[GoogleCodeExporter]

I don't know what the slowdown will be until I implement it.  :)  Maybe it 
could be an off-by-default option, so if you get bad stacks, you rerun with it, 
and get good stacks.

Original comment by rnk@google.com on 4 Aug 2012 at 12:58

[GoogleCodeExporter]

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

• Added labels: ProjectAddressSanitizer

[GoogleCodeExporter]

Adding Project:AddressSanitizer as part of GitHub migration.

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