Page MenuHomePhabricator

[sanitizer] intercept bstring functions
AcceptedPublic

Authored by kcwu on Dec 11 2016, 1:53 AM.

Details

Reviewers
kcc
dvyukov
Summary

Make all sanitizers intercept bzero, bcopy, and bcmp. Despite these functions are deprecated, they are quite popular in legacy codes.

https://github.com/google/sanitizers/issues/750

Diff Detail

Event Timeline

kcwu updated this revision to Diff 81016.Dec 11 2016, 1:53 AM
kcwu retitled this revision from to [sanitizer] intercept bstring functions.
kcwu updated this object.
kcwu added a reviewer: kcc.
kcwu added a subscriber: llvm-commits.
kcc edited edge metadata.Dec 12 2016, 10:54 AM

please add tests adjacent and similar to test/asan/TestCases/Linux/memmem_test.cc

lib/sanitizer_common/sanitizer_platform_interceptors.h
277

I think these need to be SI_LINUX, not 1.
If we try to intercept a function that does not exist on other OSes things will fail there.

ygribov added inline comments.
lib/sanitizer_common/sanitizer_platform_interceptors.h
277

Note that these functions are also available on FreeBSD and OSX.

kcwu updated this revision to Diff 81365.Dec 14 2016, 4:13 AM
kcwu edited edge metadata.

add tests and limit intercept on (Linux|FreeBSD|Mac)

kcwu updated this revision to Diff 81367.Dec 14 2016, 4:15 AM

fix typo

kcwu marked 2 inline comments as done.Dec 14 2016, 4:17 AM
kcc accepted this revision.Dec 14 2016, 10:45 AM
kcc edited edge metadata.

LGTM, and thanks a lot!
Please watch the bots as the patch is potentially risky for other platforms.

Do you have llvm commit access?
If not, I can land it.

This revision is now accepted and ready to land.Dec 14 2016, 10:45 AM
kcwu added a comment.Dec 14 2016, 10:52 AM
In D27659#622402, @kcc wrote:

Do you have llvm commit access?
If not, I can land it.

No, I don't have commit access.

kcc closed this revision.Dec 14 2016, 11:20 AM

r289690.

Note that this started causing some memory corruptions when using ASan/TSan from SVN trunk on Darwin. There's nothing wrong with this patch, but it's a binary-compatibility issue. I'll try to see if there's a workaround, but maybe I'll have to disable SANITIZER_INTERCEPT_BZERO on Darwin. I'll keep you posted.

There's nothing wrong with this patch, but it's a binary-compatibility issue

What is the issue with ABI?

There's nothing wrong with this patch, but it's a binary-compatibility issue

What is the issue with ABI?

A system library requires that bzero doesn't touch some specific register. This is true for the current system implementation of bzero, but not when using the interceptor.

kcc added a comment.Dec 15 2016, 10:39 AM

Should we change

#define SANITIZER_INTERCEPT_BZERO SI_LINUX || SI_FREEBSD || SI_MAC

to

#define SANITIZER_INTERCEPT_BZERO SI_LINUX || SI_FREEBSD

?

Should we change

If we do, please add an ugly TODO.

In D27659#623405, @kubabrecka wrote:

There's nothing wrong with this patch, but it's a binary-compatibility issue

What is the issue with ABI?

A system library requires that bzero doesn't touch some specific register. This is true for the current system implementation of bzero, but not when using the interceptor.

We think this change breaks lots of tests on our mac/asan bots (https://bugs.chromium.org/p/chromium/issues/detail?id=674435). So +1 to adding a setting for opting out of this. I'd argue that it should be off by default on Darwin until system libraries no longer make this assumption too, else asanified binaries on darwin will be broken by default.

(This currently blocks us from updating clang in chromium.)

hans added a subscriber: hans.Dec 15 2016, 12:22 PM

Reverted in r289864 so unbreak the builds until someone lands a fix.

kubamracek reopened this revision.Dec 15 2016, 12:35 PM

Re-opening since the patch was reverted. Please commit again removing SI_MAC and adding a TODO with my name:

// TODO(kuba.brecka): Add SI_MAC when strncpy on Darwin supports bzero
// interceptors. https://reviews.llvm.org/D27659.
#define SANITIZER_INTERCEPT_BZERO SI_LINUX || SI_FREEBSD

No need to change the other #defines, the issue is only with the bzero interceptor.

This revision is now accepted and ready to land.Dec 15 2016, 12:35 PM

dvyukov also says that using WRAP(memset) is a bad idea, even though we have such code in other places.

kcwu updated this revision to Diff 81705.Dec 15 2016, 6:58 PM
kcwu edited edge metadata.

SANITIZER_INTERCEPT_BZERO is changed as suggestion.

kcwu updated this revision to Diff 81707.Dec 15 2016, 7:00 PM

(rebase)

kcc added a comment.Dec 15 2016, 9:39 PM

please see internal b/33656003 about what went wrong with this patch (other than the Mac failure).
Also I've noticed that bzer_test.cc does not actually test anything -- it passes w/o the patch.
Probably you explained it here: https://github.com/google/sanitizers/issues/750#issuecomment-266262859

kcwu added a comment.Dec 16 2016, 8:46 AM
In D27659#624707, @kcc wrote:

please see internal b/33656003 about what went wrong with this patch (other than the Mac failure).
Also I've noticed that bzer_test.cc does not actually test anything -- it passes w/o the patch.
Probably you explained it here: https://github.com/google/sanitizers/issues/750#issuecomment-266262859

bzero_test.cc is easy to fix (adding -std=c99). But I am not sure how to properly fix the problem of WRAP though. Does following approach work?

Proposal: a separate CL dedicated for WRAP() issue.

  1. extract the body of "INTERCEPTOR(void*, memcpy, void *dst, const void *src, uptr size) {"

to a macro.

  1. replace all WRAP(memcpy) by the macro.
  2. repeat for others functions WRAP(*).

Any suggestions?

kcc added a comment.Dec 16 2016, 9:54 AM
In D27659#625093, @kcwu wrote:
In D27659#624707, @kcc wrote:

please see internal b/33656003 about what went wrong with this patch (other than the Mac failure).
Also I've noticed that bzer_test.cc does not actually test anything -- it passes w/o the patch.
Probably you explained it here: https://github.com/google/sanitizers/issues/750#issuecomment-266262859

bzero_test.cc is easy to fix (adding -std=c99). But I am not sure how to properly fix the problem of WRAP though. Does following approach work?

Proposal: a separate CL dedicated for WRAP() issue.

  1. extract the body of "INTERCEPTOR(void*, memcpy, void *dst, const void *src, uptr size) {" to a macro.
  2. replace all WRAP(memcpy) by the macro.
  3. repeat for others functions WRAP(*). Any suggestions?

Yes, this should work.
I would actually suggest that you do this only for the functions you change now.
If we need to fix the older interceptors, it's our yak to shave.

What's the said problem with wrapping btw?

kcc added a comment.Dec 16 2016, 10:32 AM

What's the said problem with wrapping btw?

First, it creates yet another frame. and may cause confusing stack traces.
Second, it adds a call to memset to the tsan run-time, and we have a test that checks for absence of such calls.