This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Move stat/__xstat to the common interceptors
ClosedPublic

Authored by zhaoqin on May 3 2016, 10:33 AM.

Details

Summary

Adds stat/__xstat to the common interceptors.

Removes the now-duplicate stat/__xstat interceptor from msan/tsan/esan.
This adds stat/__xstat to asan, which previously did not intercept it.

Diff Detail

Repository
rL LLVM

Event Timeline

zhaoqin updated this revision to Diff 56021.May 3 2016, 10:33 AM
zhaoqin retitled this revision from to [sanitizer] Move stat/__xstat to the common interceptors.
zhaoqin updated this object.
zhaoqin added a reviewer: aizatsky.
zhaoqin set the repository for this revision to rL LLVM.
zhaoqin added subscribers: bruening, kcc, eugenis and 2 others.
zhaoqin updated this object.May 3 2016, 10:34 AM
aizatsky added inline comments.May 3 2016, 11:15 AM
lib/sanitizer_common/sanitizer_common_interceptors.inc
5520 ↗(On Diff #56021)

The direction of this CL is good. I worry that stat/xstat behavior is not really clear.

Other interceptors (e.g. SANITIZER_INTERCEPT_STRCHR) are completely enabled/disabled by a guard. In this case it is not fully enabled/disabled, but an implementation is switched.

I suggest having two guards: one that turns it on/off, the other one chooses the implementation. WDYT?

eugenis added inline comments.May 3 2016, 11:24 AM
lib/sanitizer_common/sanitizer_common_interceptors.inc
5520 ↗(On Diff #56021)

These are actually independent interceptors, use
SANITIZER_INTERCEPT_STAT
and
SANITIZER_INTERCEPT___XSTAT

lib/sanitizer_common/sanitizer_platform_interceptors.h
40 ↗(On Diff #56021)

This is not used anywhere.
Just use !SI_FREEBSD instead, it is shorter.

zhaoqin updated this revision to Diff 56041.May 3 2016, 11:54 AM
zhaoqin marked 3 inline comments as done.

PTAL

aizatsky added inline comments.May 3 2016, 11:58 AM
lib/sanitizer_common/sanitizer_platform_interceptors.h
300 ↗(On Diff #56041)

It looks like both stat & xstat will be intercepted on mac & android.

Should this be !SANITIZER_INTERCEPT_STAT?

zhaoqin added inline comments.May 3 2016, 12:07 PM
lib/sanitizer_common/sanitizer_platform_interceptors.h
300 ↗(On Diff #56041)

If so, I am not sure if we need SANITIZER_INTERCEPT___XSTAT.
I thought SANITIZER_INTERCEPT___XSTAT is !SANITIZER_INTERCEPT_STAT, but Evgeniy suggested to add SANITIZER_INTERCEPT___XSTAT separately.
Evgeniy could you please comment on this?

zhaoqin added inline comments.May 3 2016, 12:16 PM
lib/sanitizer_common/sanitizer_common_interceptors.inc
5527 ↗(On Diff #56041)

should I add flags check here too?
I feel that the write cannot be skipped for correctness, e.g., msan mark buffer as defined.

It looks like SANITIZER_INTERCEPT___XSTAT is !SANITIZER_INTERCEPT_STAT.

This is in line with other common interceptors. Looking at sanitizer_platform_interceptors.h, one can easily figure out which functions are intercepted on any platform.

eugenis added inline comments.May 3 2016, 12:29 PM
lib/sanitizer_common/sanitizer_common_interceptors.inc
5527 ↗(On Diff #56041)

Write is necessary for msan.
Ideally, if intercept_stat==0, MSan should do WRITE_RANGE, and other sanitizers should do nothing.
Maybe these flags should be handled differently. MSan already has logic for disabling checks in nested interceptors, see MSanInterceptorContext::in_interceptor_scope. Setting that to true disables all checks, but keeps writes (i.e. msan_unpoison calls) enabled. If something like that is implemented in other sanitizers, then intercept_stat could just set this (or similar) flag in COMMON_INTERCEPTOR_ENTER.

For now just keep the WRITE_RANGE unconditionally.

zhaoqin updated this revision to Diff 56044.May 3 2016, 12:32 PM
zhaoqin updated this revision to Diff 56046.May 3 2016, 12:36 PM
eugenis accepted this revision.May 3 2016, 12:38 PM
eugenis added a reviewer: eugenis.

LGTM

This revision is now accepted and ready to land.May 3 2016, 12:38 PM

I do not have the commit right yet.
Please help me commit the CL if you think it is ready to commit, .
Thanks!

This revision was automatically updated to reflect the committed changes.
aizatsky edited edge metadata.May 3 2016, 2:49 PM

Qin,

It probably broke windows bot:

http://lab.llvm.org:8011/builders/sanitizer-windows/builds/21425/steps/build%20compiler-rt/logs/stdio

Please acknowledge and I'll rollback.

zhaoqin updated this revision to Diff 56068.May 3 2016, 3:03 PM
zhaoqin edited edge metadata.

Fixed build error on Windows