Page MenuHomePhabricator

[sanitizer] Add strlen to the common interceptors
ClosedPublic

Authored by bruening on Mar 9 2016, 5:43 PM.

Details

Summary

Adds strlen to the common interceptors, under a new common flag
intercept_strlen. This provides better sharing of interception code among
sanitizers and cleans up the inconsistent type declarations of the
previously duplicated interceptors.

Removes the now-duplicate strlen interceptor from asan, msan, and tsan.
The entry check semantics are normalized now for msan and asan, whose
private strlen interceptors contained multiple layers of checks that
included impossible-to-reach code. The new semantics are identical to the
old: bypass interception if in the middle of init or if both on Mac and not
initialized; else, call the init routine and proceed.

Preserves asan's prior replace_str flag semantics by disabling
intercept_strlen when replace_str is disabled.

Diff Detail

Repository
rL LLVM

Event Timeline

bruening updated this revision to Diff 50219.Mar 9 2016, 5:43 PM
bruening retitled this revision from to [sanitizer] Add strlen to the common interceptors.
bruening updated this object.
bruening added a reviewer: samsonov.
bruening added subscribers: zhaoqin, kcc, llvm-commits.
samsonov edited edge metadata.Mar 10 2016, 2:29 PM

Looks mostly good, thank you for working on this.

lib/asan/asan_flags.cc
162 ↗(On Diff #50219)

Hm... I think I would prefer to break the old (non-default) use case rather than have this ugly workaround... As I understand it, we will gradually phase out ASAN_OPTIONS=replace_str flag anyway, so maybe we can get away with a warning instead:

if (!f->replace_str && common_flags()->intercept_strlen) {
  Report("WARNING: strlen interceptor is enabled even though replace_str=0. Use intercept_strlen=0 to disable it.");
}
lib/asan/asan_interceptors.cc
151 ↗(On Diff #50219)

Probably you can omit this comment: I believe there are more cases when libc function may be called while __asan_init is still running. E.g. it may be required for FreeBSD: see https://llvm.org/svn/llvm-project/compiler-rt/trunk@222885

lib/sanitizer_common/sanitizer_common_interceptors.inc
205 ↗(On Diff #50219)

I don't think you need it here: looks like this will be handled by COMMON_INTERCEPTOR_ENTER macro.

211 ↗(On Diff #50219)

I think it should be just COMMON_INTERCEPTOR_READ_RANGE - COMMON_INTERCEPTOR_READ_STRING is only used for the special-handling of cases where we are accessing only some prefix of a null-terminated string.

bruening updated this revision to Diff 50372.Mar 10 2016, 3:59 PM
bruening marked 4 inline comments as done.
bruening edited edge metadata.

Removes the common flags override and makes other changes as suggested by
the review.

lib/asan/asan_flags.cc
162 ↗(On Diff #50219)

Sure, I needed feedback on this one: did not want to break backward compatibility without a project owner signing off.

lib/sanitizer_common/sanitizer_common_interceptors.inc
205 ↗(On Diff #50219)

Some common interceptors have their own init check which is what this is based on, but most don't and it does seem to be superfluous. Perhaps I will send send a separate CL removing the existing checks.

samsonov accepted this revision.Mar 10 2016, 4:22 PM
samsonov edited edge metadata.

LGTM, thank you!

This revision is now accepted and ready to land.Mar 10 2016, 4:22 PM
vitalybuka accepted this revision.Mar 10 2016, 4:24 PM
vitalybuka edited edge metadata.

As before, could you commit the patch for me? Thank you.

This revision was automatically updated to reflect the committed changes.
bruening added inline comments.Mar 24 2016, 1:29 PM
lib/sanitizer_common/sanitizer_common_interceptors.inc
205 ↗(On Diff #50219)

Note that this is not quite the same as the COMMON_INTERCEPTOR_ENTER, because REAL(strlen) is not initialized until InitializeCommonInterceptors() is called (on Linux at least). Thus any use of strlen prior to that point needs to use internal_strlen(), not REAL(strlen). Given that nothing broke since this was committed, perhaps we can assume that nothing needs strlen that early.