Page MenuHomePhabricator

Specify which sanitizers are covered by a sanitizer blacklist
Needs ReviewPublic

Authored by vsk on May 3 2017, 5:25 PM.

Details

Summary

Sanitizer blacklists currently apply to all enabled sanitizers. E.g if
multiple sanitizers are enabled, it isn't possible to specify a
blacklist for one sanitizer and not another.

This makes it impossible to load default blacklists for more than one
sanitizer, because doing so would result in false negatives (i.e, some
code may not be instrumented by the right sanitizer, because the final
sanitizer blacklist is the union of all available blacklists).

This patch fixes the situation by changing the internal representation
of blacklists. It becomes mandatory to specify which sanitizers are
covered by each blacklist, and to specify the sanitization kind up-front
when querying the blacklist.

This resolves the "multiple default sanitizer blacklists" situation.
Instead of arbitrarily picking a default blacklist and ignoring the
rest, we would correctly load all of them s.t entries in one blacklist
which apply to one sanitizer could not mistakenly be applied to another
sanitizer.

For now, the user-facing -fsanitize-blacklist driver option is left
unchanged. Specifying a blacklist in this way creates a blacklist which
covers all enabled sanitizers.

The internal -fsanitize-blacklist frontend option is modified so that
the names of the covered sanitizers are passed along with the path to
the blacklist file. E.g this:

clang -cc1 -fsanitize=address -fsanitize-blacklist=BL.txt

Becomes this:

clang -cc1 -fsanitize=address -fsanitize-blacklist=address:BL.txt

This patch obsoletes D32047.

Diff Detail

Event Timeline

vsk created this revision.May 3 2017, 5:25 PM
vsk added a subscriber: zaks.anna.

@zaks.anna suggested some more reviewers who may be interested.

Ping. (@george.karpenkov, do you have the time to take a look?)

@vsk Looks reasonable to me, although I would say such a change should probably require more tests.
However, I am not a C++ expert, and I am still not overly familiar with a codebase, so I'm not sure whether I should be a single reviewer.

eugenis edited edge metadata.May 30 2017, 1:23 PM

This change scares me a bit, to be honest. Is this really that big of a problem? Blacklists are not supposed to be big, maybe we can tolerate a few false negatives?

Consider extending the blacklist syntax instead, the same way Valgrind does. Blacklist entries could be annotated with a list of sanitizers they apply to, like

asan,ubsan : src: file.cc:line

or an even less verbose way using sections

[asan]
src:
src:
[msan]
src:

As an extra benefit, all default blacklists can be merged into one.

vsk added a comment.May 30 2017, 2:03 PM

This change scares me a bit, to be honest. Is this really that big of a problem? Blacklists are not supposed to be big, maybe we can tolerate a few false negatives?

I'd like to make it possible to deploy a larger default blacklist for one sanitizer without inhibiting the other sanitizers. This would be useful if we find a bug in a runtime check: we could temporarily work around the issue by deploying a new blacklist, instead of changing the compiler or build system. It won't be possible to do this if blacklist updates can introduce false negatives.

Also, as a separate point, I think a design which permits false negatives is worrisome in and of itself, and is worth fixing.

Consider extending the blacklist syntax instead, the same way Valgrind does.

I like this idea. However, I think that it would require most of the changes in this patch, with some additional work to change SpecialCaseList.

Blacklist entries could be annotated with a list of sanitizers they apply to, like

asan,ubsan : src: file.cc:line

or an even less verbose way using sections

[asan]
src:
src:
[msan]
src:

As an extra benefit, all default blacklists can be merged into one.

It would be nice to combine the default blacklists into one file with separate sections for each sanitizer. I'll look into this (hopefully next week).

Thanks. Can you update "SanitizerArgs::collectDefaultBlacklists" to include "ubsan_blacklist.txt"?

Ping? Can we make a decision on this?
I've this simple one D35849: [UBSan] Provide default blacklist filename for UBSan which, depending on this, shall be discarded or move forward.
If this CL stalls, I'll seek to proceed with D35849. Any how, D35849 gets wiped out whence this CL (D32842) is submitted.

vsk added a comment.Jul 31 2017, 12:50 PM

Ping? Can we make a decision on this?
I've this simple one D35849: [UBSan] Provide default blacklist filename for UBSan which, depending on this, shall be discarded or move forward.
If this CL stalls, I'll seek to proceed with D35849. Any how, D35849 gets wiped out whence this CL (D32842) is submitted.

Please go ahead with D35849, I believe @pcc sgtm'd an identical patch of mine a while back, and it lgtm as well. Once I have time to revisit this I'll make sure to consider ubsan_blacklist.txt.

vsk updated this revision to Diff 114882.Sep 12 2017, 12:50 PM
  • Rebase to ToT.
vsk added a comment.Sep 12 2017, 12:55 PM

@eugenis I gave the idea of annotating blacklist entries with a list of sanitizers they apply to some thought. It would be a nice follow-up to this change, but would depend on it. As an initial step, I think we should move forward with this patch, since it makes it possible to apply blacklists selectively.

@vsk Why don't I take a look at implementing the blacklist selection methods @eugenis mentioned on top of this change now so that we can skip ahead and merge something everyone is satisfied with?

vsk added a comment.Sep 12 2017, 1:53 PM

@vsk Why don't I take a look at implementing the blacklist selection methods @eugenis mentioned on top of this change now so that we can skip ahead and merge something everyone is satisfied with?

Feel free.