dr asan inlined strlen false positive in libfontconfig.so

[ramosian-glider]

Originally reported on Google Code with ID 97

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.

Reported by rnk@google.com on 2012-08-02 20:05:34

[ramosian-glider]

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.

Reported by ramosian.glider on 2012-08-03 08:26:47

[ramosian-glider]

+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 :(

Reported by timurrrr@google.com on 2012-08-03 08:51:03

[ramosian-glider]

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.

Reported by rnk@google.com on 2012-08-03 14:28:42

[ramosian-glider]

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" ?

Reported by timurrrr@google.com on 2012-08-03 16:38:56

[ramosian-glider]

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

Reported by thestig@google.com on 2012-08-03 16:43:57

[ramosian-glider]

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)?

Reported by ramosian.glider on 2012-08-03 16:46:55

[ramosian-glider]

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

Reported by timurrrr@google.com on 2012-08-03 16:50:08

[ramosian-glider]

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.

Reported by rnk@google.com on 2012-08-03 17:23:22

[ramosian-glider]

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

Reported by timurrrr@google.com on 2012-08-03 18:04:11

[ramosian-glider]

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.

Reported by rnk@google.com on 2012-08-03 19:24:40

[ramosian-glider]

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

Reported by timurrrr@google.com on 2012-08-04 08:06:10

[ramosian-glider]

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.

Reported by rnk@google.com on 2012-08-04 12:58:45

[ramosian-glider]

Reported by ramosian.glider on 2015-07-30 09:05:30

• Labels added: ProjectAddressSanitizer

[ramosian-glider]

Adding Project:AddressSanitizer as part of GitHub migration.

Reported by ramosian.glider on 2015-07-30 09:06:55