Page MenuHomePhabricator

Stop intercepting mallinfo and mallopt on FreeBSD
ClosedPublic

Authored by dim on Dec 10 2016, 8:33 AM.

Details

Summary

In https://bugs.freebsd.org/215125 I was notified that some configure
scripts attempt to test for the Linux-specific mallinfo and mallopt
functions by compiling and linking small programs which references the
functions, and observing whether that results in errors.

On FreeBSD there are neither mallinfo nor mallopt functions, so
normally these tests would fail, but when sanitizers are enabled, they
incorrectly succeed, because the sanitizers define interceptors for
these functions.

Fix this by not intercepting mallinfo and mallopt for FreeBSD, in
all of the sanitizers.

Diff Detail

Repository
rL LLVM

Event Timeline

dim updated this revision to Diff 81002.Dec 10 2016, 8:33 AM
dim retitled this revision from to Stop intercepting mallinfo and mallopt on FreeBSD.
dim updated this object.
dim added reviewers: kcc, emaste.
dim added a subscriber: llvm-commits.
kcc edited edge metadata.Dec 12 2016, 11:16 AM

Please add a test.
I wonder if it's possible to have fewer ifdefs?

lib/asan/asan_malloc_linux.cc
117 ↗(On Diff #81002)

Not like this.
Please add
#define SANITIZER_INTERCEPT_MALLOPT_AND_MALLINFO (!SANITIZER_FREEBSD)
to lib/sanitizer_common/sanitizer_platform_interceptors.h
then use it here.

dim added a comment.Dec 12 2016, 11:20 AM
In D27654#620145, @kcc wrote:

Please add a test.

Hm, what kind of test did you have in mind? It is not useful to test for mallinfo() or mallopt() on FreeBSD, since they never existed in the first place.

I wonder if it's possible to have fewer ifdefs?

I don't think so, because the symbols should not be defined at all by the sanitizer on FreeBSD. If we just add empty wrappers, those will still be picked up by the linker.

Basically the #if statements would now refer to SANITIZER_INTERCEPT_MALLOPT_AND_MALLINFO.

kcc added a comment.Dec 12 2016, 11:22 AM

a test would be FreeBSD-specific and it would verify that mallinfo/mallopt are not present.

also, please add coments after #ifdef

#ifdef // SANITIZER_INTERCEPT_MALLOPT_AND_MALLINFO

dim updated this revision to Diff 81118.Dec 12 2016, 11:38 AM
dim edited edge metadata.

First round of changes, no specific test yet, only the adding of
SANITIZER_INTERCEPT_MALLOPT_AND_MALLINFO.

dim updated this revision to Diff 81884.Dec 18 2016, 8:56 AM

Rebase.

dim updated this revision to Diff 81888.Dec 18 2016, 1:02 PM

Add a very simple XFAIL testcase for FreeBSD. I am unsure on how to
test for 'negative' results, ocurring on specific OSes, so this is the
only thing I could come up with. The test case should link fine on
Linux, as far as I can see. Suggestions for improvements welcome! :)

dim added a comment.Dec 20 2016, 12:11 AM

@kcc is this OK now? I'd like to commit a similar fix to FreeBSD's copy of compiler-rt, but I'd rather use the same one as submitted here. :)

kcc added inline comments.Dec 27 2016, 3:54 PM
test/asan/TestCases/mallinfo-mallopt.cpp
4 ↗(On Diff #81888)

hm. is freebsd the only platform where this will fail?
What about other non-linux OSes?

joerg added a subscriber: joerg.Dec 27 2016, 4:15 PM

I'm not sure if any platform beside glibc supports mallinfo/mallopt.

dim added inline comments.Dec 31 2016, 4:53 PM
test/asan/TestCases/mallinfo-mallopt.cpp
4 ↗(On Diff #81888)

Indeed, probably better to make this test succeed on Linux only. Is there any way to express XFAIL: !linux ?

dim updated this revision to Diff 83560.Jan 8 2017, 7:25 AM

Rebased again.

dim added a comment.Jan 8 2017, 7:45 AM

I'd like to commit this now so it ends up in 4.0.0. Any other suggestions?

joerg added inline comments.Jan 8 2017, 9:02 AM
test/asan/TestCases/mallinfo-mallopt.cpp
7 ↗(On Diff #83560)

Can you make those proper protypes?

4 ↗(On Diff #81888)

My suggestion would be to do conditionalize the main function, i.e.

#if __glibc__
  mallinfo();
  mallopt();
#else
  void non_existant_function(void);
  non_existant_function();
#endif

and just check for linking error on all platforms.

dim added inline comments.Jan 8 2017, 9:53 AM
test/asan/TestCases/mallinfo-mallopt.cpp
7 ↗(On Diff #83560)

This is not what autoconf is doing, which was the original inspiration for this change. Autoconf always uses function declarations without any parameters, and I was mimicking what they are doing.

4 ↗(On Diff #81888)

Actually, the error we're checking for is that mallinfo() and mallopt() are *not* incorrectly intercepted. So we have to specifically attempt to link these functions in, and see if those don't cause any error, and if they don't, there is a regression.

joerg added inline comments.Jan 8 2017, 10:07 AM
test/asan/TestCases/mallinfo-mallopt.cpp
7 ↗(On Diff #83560)

Yeah, but that part is one of the design flaws in autoconf. I guess the problem is that declaring mallinfo correctly would need the content of the struct, so that's kind of not possible in a good way without depending on the header.

4 ↗(On Diff #81888)

Sorry, inverted condition. I meant '#if !glibc'. That should make it clearer. My point is to link against something explicitly undefined for glibc and just make sure that it always fails that way.

dim added inline comments.Jan 9 2017, 12:03 AM
test/asan/TestCases/mallinfo-mallopt.cpp
7 ↗(On Diff #83560)

Indeed, I find autoconf's style ugly too, but this avoids having to include any headers.

4 ↗(On Diff #81888)

Ah yes, I see what you mean.

dim added inline comments.Jan 9 2017, 2:27 AM
test/asan/TestCases/mallinfo-mallopt.cpp
4 ↗(On Diff #81888)

Hm, though glibc is only defined by including one of the glibc headers, I assume? I'll have to look it up on a Linux system.

dim updated this revision to Diff 84172.Jan 12 2017, 1:54 PM
  • Test that mallinfo() and mallopt() cause link failures on non-glibc platforms
  • Use nonexistent function names on glibc platforms, to also cause link failures
  • XFAIL on all platforms
  • Use proper prototypes
dim updated this revision to Diff 84184.Jan 12 2017, 3:26 PM
  • Stop intercepting more functions that don't exist on FreeBSD/Mac: cfree, memalign, pvalloc
  • Use macros similar to TSan for InitializeInterceptors to reduce #ifdef soup
  • Handle all not-to-be intercepted functions with individual compile/link invocations
joerg added a comment.Jan 13 2017, 3:56 AM

Test case looks good now. Should the *.h definitions use GLIBC or at least LINUX_NOT_ANDROID too?

dim added a comment.EditedJan 13 2017, 4:17 AM

Test case looks good now. Should the *.h definitions use GLIBC or at least LINUX_NOT_ANDROID too?

You mean in sanitizer_platform_interceptors.h? I am unsure what Android supports, so I want to leave that to somebody else with knowledge of that platform. The definitions I added just try to follow the style in the rest of sanitizer_platform_interceptors.h.

Additionally, I think __GLIBC__ is only defined when you include certain glibc headers, and I think sanitizer_platform_interceptors.h wants to avoid including any standard headers.

dim added a subscriber: hans.Jan 27 2017, 1:03 PM

Ping? I'd really like to get this into 4.0.0, if at all possible. @hans I think there is still time enough now, right?

kcc accepted this revision.Jan 27 2017, 1:06 PM

LGTM, assuming tests pass on other platforms (please check at least one other platform, then watch the bots after commit)

This revision is now accepted and ready to land.Jan 27 2017, 1:06 PM
dim added a comment.Jan 27 2017, 2:00 PM

It's pretty weird, on Ubuntu, the lsan changes make exactly one lsan test case fail, with:

FAIL: LeakSanitizer-Standalone-x86_64 :: TestCases/use_tls_dynamic.cc (30686 of 31782)
******************** TEST 'LeakSanitizer-Standalone-x86_64 :: TestCases/use_tls_dynamic.cc' FAILED ********************
Script:
--
LSAN_BASE="report_objects=1:use_stacks=0:use_registers=0:use_ld_allocations=0"
/home/ubuntu/obj/llvm-293332-trunk-linux4-x86_64-ninja-rel-1/./bin/clang --driver-mode=g++ -O0 -m64 -gline-tables-only /home/ubuntu/src/llvm/trunk/projects/compiler-rt/test/lsan/TestCases/use_tls_dynamic.cc -DBUILD_DSO -fPIC -shared -o /home/ubuntu/obj/llvm-293332-trunk-linux4-x86_64-ninja-rel-1/projects/compiler-rt/test/lsan/X86_64LsanConfig/TestCases/Output/use_tls_dynamic.cc.tmp-so.so
/home/ubuntu/obj/llvm-293332-trunk-linux4-x86_64-ninja-rel-1/./bin/clang --driver-mode=g++ -O0 -m64 -gline-tables-only -fsanitize=leak -I/home/ubuntu/src/llvm/trunk/projects/compiler-rt/test/lsan/../ /home/ubuntu/src/llvm/trunk/projects/compiler-rt/test/lsan/TestCases/use_tls_dynamic.cc -o /home/ubuntu/obj/llvm-293332-trunk-linux4-x86_64-ninja-rel-1/projects/compiler-rt/test/lsan/X86_64LsanConfig/TestCases/Output/use_tls_dynamic.cc.tmp
LSAN_OPTIONS=$LSAN_BASE:"use_tls=0" not  /home/ubuntu/obj/llvm-293332-trunk-linux4-x86_64-ninja-rel-1/projects/compiler-rt/test/lsan/X86_64LsanConfig/TestCases/Output/use_tls_dynamic.cc.tmp 2>&1 | FileCheck /home/ubuntu/src/llvm/trunk/projects/compiler-rt/test/lsan/TestCases/use_tls_dynamic.cc
LSAN_OPTIONS=$LSAN_BASE:"use_tls=1"  /home/ubuntu/obj/llvm-293332-trunk-linux4-x86_64-ninja-rel-1/projects/compiler-rt/test/lsan/X86_64LsanConfig/TestCases/Output/use_tls_dynamic.cc.tmp 2>&1
LSAN_OPTIONS=""  /home/ubuntu/obj/llvm-293332-trunk-linux4-x86_64-ninja-rel-1/projects/compiler-rt/test/lsan/X86_64LsanConfig/TestCases/Output/use_tls_dynamic.cc.tmp 2>&1
--
Exit Code: 23

Command Output (stdout):
--
Test alloc: 0x61a000000600

=================================================================
==7609==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1337 byte(s) in 1 object(s) allocated from:
    #0 0x424445 in __interceptor_malloc /home/ubuntu/src/llvm/trunk/projects/compiler-rt/lib/lsan/lsan_interceptors.cc:55:3
    #1 0x425db5 in main /home/ubuntu/src/llvm/trunk/projects/compiler-rt/test/lsan/TestCases/use_tls_dynamic.cc:26:13
    #2 0x7f8d7020182f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

Objects leaked above:
0x61a000000600 (1337 bytes)

SUMMARY: LeakSanitizer: 1337 byte(s) leaked in 1 allocation(s).

--

I'm not completely clear on what is going wrong here... Maybe some interceptors get incorrectly disabled on Linux? And this causes a leak?

dim updated this revision to Diff 86119.Jan 27 2017, 2:18 PM

Updated to include sanitizer_platform_interceptors.h in lsan_interceptors.cc, so it correctly picks up the SANITIZER_INTERCEPT_xxx defines.

Tested on FreeBSD and Ubuntu.

kcc requested changes to this revision.Jan 27 2017, 2:19 PM

LGTM, please watch the bots

This revision now requires changes to proceed.Jan 27 2017, 2:19 PM
This revision was automatically updated to reflect the committed changes.
hans added a comment.Jan 27 2017, 2:44 PM
In D27654#659137, @dim wrote:

Ping? I'd really like to get this into 4.0.0, if at all possible. @hans I think there is still time enough now, right?

Yes. I see this just landed. Let me know when it's been exercised by the bots again.

dim added a comment.Jan 28 2017, 4:44 AM

Ugh, apparently not does not work on Windows, or at least not the way I would expect it to:

FAIL: AddressSanitizer-i386-windows :: TestCases/malloc-no-intercept.c (502 of 562)
******************** TEST 'AddressSanitizer-i386-windows :: TestCases/malloc-no-intercept.c' FAILED ********************
Script:
--
not   C:/b/slave/sanitizer-windows/build/./bin/clang.exe -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -gcodeview  -fms-compatibility-version=19.00.24215.1 -Dtestfunc=mallinfo C:\b\slave\sanitizer-windows\llvm\projects\compiler-rt\test\asan\TestCases\malloc-no-intercept.c -o C:\b\slave\sanitizer-windows\build\projects\compiler-rt\test\asan\I386WindowsConfig\TestCases\Output\malloc-no-intercept.c.tmp
not   C:/b/slave/sanitizer-windows/build/./bin/clang.exe -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -gcodeview  -fms-compatibility-version=19.00.24215.1 -Dtestfunc=mallopt  C:\b\slave\sanitizer-windows\llvm\projects\compiler-rt\test\asan\TestCases\malloc-no-intercept.c -o C:\b\slave\sanitizer-windows\build\projects\compiler-rt\test\asan\I386WindowsConfig\TestCases\Output\malloc-no-intercept.c.tmp
not   C:/b/slave/sanitizer-windows/build/./bin/clang.exe -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -gcodeview  -fms-compatibility-version=19.00.24215.1 -Dtestfunc=memalign C:\b\slave\sanitizer-windows\llvm\projects\compiler-rt\test\asan\TestCases\malloc-no-intercept.c -o C:\b\slave\sanitizer-windows\build\projects\compiler-rt\test\asan\I386WindowsConfig\TestCases\Output\malloc-no-intercept.c.tmp
not   C:/b/slave/sanitizer-windows/build/./bin/clang.exe -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -gcodeview  -fms-compatibility-version=19.00.24215.1 -Dtestfunc=pvalloc  C:\b\slave\sanitizer-windows\llvm\projects\compiler-rt\test\asan\TestCases\malloc-no-intercept.c -o C:\b\slave\sanitizer-windows\build\projects\compiler-rt\test\asan\I386WindowsConfig\TestCases\Output\malloc-no-intercept.c.tmp
not   C:/b/slave/sanitizer-windows/build/./bin/clang.exe -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -gcodeview  -fms-compatibility-version=19.00.24215.1 -Dtestfunc=cfree    C:\b\slave\sanitizer-windows\llvm\projects\compiler-rt\test\asan\TestCases\malloc-no-intercept.c -o C:\b\slave\sanitizer-windows\build\projects\compiler-rt\test\asan\I386WindowsConfig\TestCases\Output\malloc-no-intercept.c.tmp
--
Exit Code: 1

Command Output (stdout):
--
$ "not" "C:/b/slave/sanitizer-windows/build/./bin/clang.exe" "-fsanitize=address" "-mno-omit-leaf-frame-pointer" "-fno-omit-frame-pointer" "-fno-optimize-sibling-calls" "-gline-tables-only" "-gcodeview" "-fms-compatibility-version=19.00.24215.1" "-Dtestfunc=mallinfo" "C:\b\slave\sanitizer-windows\llvm\projects\compiler-rt\test\asan\TestCases\malloc-no-intercept.c" "-o" "C:\b\slave\sanitizer-windows\build\projects\compiler-rt\test\asan\I386WindowsConfig\TestCases\Output\malloc-no-intercept.c.tmp"
# command output:
   Creating library C:\b\slave\sanitizer-windows\build\projects\compiler-rt\test\asan\I386WindowsConfig\TestCases\Output\malloc-no-intercept.c.lib and object C:\b\slave\sanitizer-windows\build\projects\compiler-rt\test\asan\I386WindowsConfig\TestCases\Output\malloc-no-intercept.c.exp

malloc-no-intercept-f1349f.o : error LNK2019: unresolved external symbol _mallinfo referenced in function _main

C:\b\slave\sanitizer-windows\build\projects\compiler-rt\test\asan\I386WindowsConfig\TestCases\Output\malloc-no-intercept.c.tmp : fatal error LNK1120: 1 unresolved externals


# command stderr:
clang.exe: error: linker command failed with exit code 1120 (use -v to see invocation)

E.g the test cases are *supposed* to fail to to link, and in this case link failure means the test succeeded. @hans, any clue on this? Or should I just XFAIL this test on Windows?

dim reopened this revision.Jan 28 2017, 11:19 AM
This revision now requires changes to proceed.Jan 28 2017, 11:19 AM
dim updated this revision to Diff 86182.Jan 28 2017, 11:20 AM
dim edited edge metadata.

Trying to fix the test case for Windows.

Apparently lib/asan/asan_malloc_win.cc has always had a wrapper
cfree() implementation, added by @timurrr. ASan should not intercept
or provide that function, otherwise configuration tools such as cmake or
autoconf can incorrectly detect that cfree is valid and supported.

Let's just get rid of the wrapper. I think this will fix the build bots
for Windows.

Adding timurrrr since he originally added the cfree wrapper.

timurrrr edited reviewers, added: rnk; removed: timurrrr.Jan 30 2017, 2:23 AM
timurrrr added a subscriber: timurrrr.

Sorry, it's been a long time since I've worked on this code and I don't remember anything.
Reid, please review or help find a better reviewer.

Also be careful if you proceed with removing the cfree wrapper as it may have unexpected consequences on Windows.

dim added a comment.Jan 30 2017, 3:36 AM

Also be careful if you proceed with removing the cfree wrapper as it may have unexpected consequences on Windows.

Interesting. Since I'm not that familiar with Windows development anymore, what kind of things could go wrong? If it's too much hassle I can simply undo the removal, and get rid of the cfree test for Windows.

rnk edited edge metadata.Jan 30 2017, 10:16 AM
In D27654#660232, @dim wrote:

Also be careful if you proceed with removing the cfree wrapper as it may have unexpected consequences on Windows.

Interesting. Since I'm not that familiar with Windows development anymore, what kind of things could go wrong? If it's too much hassle I can simply undo the removal, and get rid of the cfree test for Windows.

The implementation of winasan cfree just aborts. I'd delete it and see if the bots like it.

dim added a comment.Jan 30 2017, 10:23 AM
In D27654#660686, @rnk wrote:
In D27654#660232, @dim wrote:

Also be careful if you proceed with removing the cfree wrapper as it may have unexpected consequences on Windows.

Interesting. Since I'm not that familiar with Windows development anymore, what kind of things could go wrong? If it's too much hassle I can simply undo the removal, and get rid of the cfree test for Windows.

The implementation of winasan cfree just aborts. I'd delete it and see if the bots like it.

Yes, I did just that in the last update of this diff. I hope that nobody minds if I break the bots twice :)

This revision was automatically updated to reflect the committed changes.