This is an archive of the discontinued LLVM Phabricator instance.

tsan: ignore interceptors coming from specified libraries
ClosedPublic

Authored by dvyukov on Oct 2 2013, 7:03 AM.

Details

Summary

LibIgnore allows to ignore all interceptors called from a particular set
of dynamic libraries. LibIgnore remembers all "called_from_lib" suppressions
from the provided SuppressionContext; finds code ranges for the libraries;
and checks whether the provided PC value belongs to the code ranges.

Also make malloc and friends interceptors use SCOPED_INTERCEPTOR_RAW instead of SCOPED_TSAN_INTERCEPTOR, because if they are called from an ignored lib, then must call our internal allocator instead of libc malloc.

Diff Detail

Event Timeline

dvyukov updated this revision to Unknown Object (????).Oct 2 2013, 7:08 AM
samsonov added inline comments.Oct 2 2013, 7:34 AM
sanitizer_common/sanitizer_libignore.h
42

Consider reusing LoadedModule from sanitizer_common.h

sanitizer_common/sanitizer_suppressions.h
47

Can we do better than return the class internals to the caller?

tsan/lit_tests/ignore_lib0.cc
2

Please use %p and %t wildcards (see asan/lit_tests/TestCases/dlclose-test.cc as an example)

5

TSAN_OPTIONS=$TSAN_OPTIONS:....

tsan/rtl/tsan_interceptors.cc
130–139
  • do you need to use ALIGNED(64) here?
  • why not static char [sizeof(LibIgnore)]?
171

in_ingored_lib_(false)

tsan/rtl/tsan_rtl.cc
217

Why not put this under a single ifndef?

kcc added a comment.Oct 2 2013, 7:37 AM

did a quick scan, more tomorrow

sanitizer_common/sanitizer_libignore.cc
22

I'd prefer to have size() and get(i) methods in supp instead of giving away the guts.

sanitizer_common/sanitizer_libignore.h
28

Will this work on Windows? (Check with Timur)

30

If this is linker-initialized, the empty body should be here. No?

tsan/rtl/tsan_interceptors.cc
130–139

shouldn't this be aligned(64)?

tsan/rtl/tsan_rtl.cc
216–217

Two #ifndef TSAN_GO in a row?

Also, can we start using if (!TSAN_GO) instead ?

glider added inline comments.Oct 2 2013, 7:40 AM
msan/msan_interceptors.cc
1133 ↗(On Diff #4600)

IIUC this file doesn't belong to this CL.
If so, please commit it separately.

sanitizer_common/sanitizer_libignore.h
36

Shouldn't we support dlclose()? Otherwise you may end up having overlapping and/or nesting ranges.

glider added inline comments.Oct 2 2013, 7:40 AM
sanitizer_common/sanitizer_common_interceptors.inc
1142 ↗(On Diff #4600)

Do these interceptors belong to this CL?

sanitizer_common/sanitizer_libignore.cc
27

I don't think we must bail out if the number of suppressions is greater than a predefined constant. Maybe just ignore the rest?

sanitizer_common/sanitizer_libignore.h
11

I suggest 'IgnoreLib' or anything else instead of 'LibIgnore', since 'libignore' associates with 'library named ignore'.

dvyukov updated this revision to Unknown Object (????).Oct 2 2013, 11:35 PM

addressed the comments

PTAL

sanitizer_common/sanitizer_libignore.cc
22

Done

sanitizer_common/sanitizer_libignore.h
28

It's not used on windows.

30

It's in cc file.

42

LoadedModule is too general and slow for this use case.

sanitizer_common/sanitizer_suppressions.h
47

Done

tsan/lit_tests/ignore_lib0.cc
2

I need to know the exact library name to put into suppressions file, I do not see how to handle this.

5

Done

tsan/rtl/tsan_interceptors.cc
130–139

do you need to use ALIGNED(64) here?

The class itself is aligned.

130–139

why not static char [sizeof(LibIgnore)]?

That does not provide necessary storage alignment -> undefined behavior.

130–139

shouldn't this be aligned(64)?

The class itself is aligned.

171

why?

tsan/rtl/tsan_rtl.cc
216–217

Two #ifndef TSAN_GO in a row?

Done

216–217

Also, can we start using if (!TSAN_GO) instead ?

There is no such function when TSAN_GO, so it may not link.

217

Done

dvyukov updated this revision to Unknown Object (????).Oct 2 2013, 11:36 PM
samsonov added inline comments.Oct 3 2013, 1:01 AM
sanitizer_common/sanitizer_libignore.h
28

It is still compiled on Windows.

tsan/lit_tests/ignore_lib0.cc
2

You can use %T wildcard for a temporary directory.

tsan/rtl/tsan_interceptors.cc
130–139

This deserves a careful comment or maybe even compiler check.

171

I like to actually see the default value (and notice that in_ignored_lib_ is a bool). Not sure if it's consistent with conventions you use in TSan code.

samsonov added inline comments.Oct 3 2013, 1:13 AM
sanitizer_common/sanitizer_libignore.cc
39

Why not InternalScopedBuffer?

45

I think that if you change the order of loops (scan ProcSelfMaps and match its contents against all libs), you can make the code more testable

49

s/Printf/Report

66

Are you sure we want to report it?

tsan/lit_tests/ignore_lib1.cc
31

Wait, is this copied from ingore_lib0.cc? I think it's better to put this source under lit_tests/Helpers directory - see the way it's done in ASan.

dvyukov updated this revision to Unknown Object (????).Oct 3 2013, 3:14 AM

addressed the comments

dvyukov added inline comments.Oct 3 2013, 3:17 AM
msan/msan_interceptors.cc
1133 ↗(On Diff #4600)

this file was removed from this CL

sanitizer_common/sanitizer_common_interceptors.inc
1142 ↗(On Diff #4600)

this file was removed from this CL

sanitizer_common/sanitizer_libignore.cc
39

Done

45

how can I make it more testable?

49

Done

66

another library will be ignored in this case, which is bad
I do not want to handle library unloading, so I just check that it does not happen

sanitizer_common/sanitizer_libignore.h
11

IgnoreLib is not much better, it's still Library if you read it this way

28

Done
Added #if SANITIZER_LINUX

36

Done
See OnLibraryUnloaded

tsan/lit_tests/ignore_lib0.cc
2

Done

tsan/lit_tests/ignore_lib1.cc
31

Done

tsan/rtl/tsan_interceptors.cc
130–139

Added a comment above the class definition.

171

Done

dvyukov updated this revision to Unknown Object (????).Oct 3 2013, 3:18 AM
dvyukov updated this revision to Unknown Object (????).Oct 3 2013, 3:42 AM

moved LibIgnore alignment from class definition to variable definition

kcc accepted this revision.Oct 3 2013, 3:58 AM

LGTM

Anybody else?

samsonov added inline comments.Oct 3 2013, 4:46 AM
sanitizer_common/sanitizer_libignore.cc
40

You don't need this array - at i-th iteration you use only loaded[i];

47

s/&fn[0]/fn.data() (here and below)

54

You may use UNREACHABLE macro here and below

tsan/lit_tests/ignore_lib0.cc
5

Why do you need this LD_LIBRARY_PATH and Output here?

dvyukov added inline comments.Oct 3 2013, 5:11 AM
sanitizer_common/sanitizer_libignore.cc
40

Done

47

Done

54

It is reachable code. Changed CHECK(0) to Die().

tsan/lit_tests/ignore_lib0.cc
5

Otherwise the library won't be loaded, dynamic linker won't be able to find it.

dvyukov updated this revision to Unknown Object (????).Oct 3 2013, 5:11 AM

addressed the comments

samsonov added inline comments.Oct 3 2013, 5:16 AM
tsan/lit_tests/ignore_lib0.cc
5

Alright... can you use %T instead of Output?

dvyukov updated this revision to Unknown Object (????).Oct 3 2013, 5:31 AM

PTAL

tsan/lit_tests/ignore_lib0.cc
5

Done

LGTM (with nits).

tsan/lit_tests/ignore_lib0.cc
5

I see one more "Output" here :)

tsan/lit_tests/ignore_lib0.cc.supp
2

FYI: If you prefer, you may add "echo called_from_lib:/libignore_lib0.so > %t.supp" to the RUN: lines in your test cases and avoid these extra files.

Added to the description:

Also make malloc and friends interceptors use SCOPED_INTERCEPTOR_RAW instead of SCOPED_TSAN_INTERCEPTOR, because if they are called from an ignored lib, then must call our internal allocator instead of libc malloc.

dvyukov updated this revision to Unknown Object (????).Oct 3 2013, 5:55 AM

PTAL

glider added inline comments.Oct 3 2013, 6:03 AM
sanitizer_common/sanitizer_libignore.cc
19

Perhaps move the ctor into the header?

30

This check can be easily triggered by the user if he adds many suppressions to the file.
Can you print a meaningful error report in this case?

49

Shouldn't '&&' be on the previous line?

52

Style nit: the quotes will look much prettier if you align them with the previous line (if you choose to do this, make sure to align the next line similarly). Ditto below.

57

(!lib->loaded) ?

sanitizer_common/sanitizer_libignore.h
16

Need a newline after this comment.

67

I suggest to remove the declarations of non-implemented functions.

tsan/lit_tests/ignore_lib1.cc
23

pls add spaces around "|" here and below

tsan/lit_tests/ignore_lib_lib.h
22

Please fix the indentation

tsan/rtl/tsan_rtl.h
35

Mind the alphabetical order of includes.

dvyukov added inline comments.Oct 3 2013, 6:16 AM
sanitizer_common/sanitizer_libignore.cc
19

Why?

30

Done

49

Done

52

Done

57

Done

sanitizer_common/sanitizer_libignore.h
16

Done

67

Then it will be possible to copy objects of this class, something that I want to prevent.

tsan/lit_tests/ignore_lib1.cc
23

Done

tsan/lit_tests/ignore_lib_lib.h
22

Done

tsan/rtl/tsan_rtl.h
35

Done

dvyukov updated this revision to Unknown Object (????).Oct 3 2013, 6:17 AM
glider added a comment.Oct 3 2013, 6:22 AM

LGTM with a nit

sanitizer_common/sanitizer_libignore.h
67

Got it now. I just think "not implemented" is a poor comment.
How about "disallow copying LibIgnore objects"?

Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in r191897.