Page MenuHomePhabricator

Allow specifying sanitizers in blacklists
ClosedPublic

Authored by vlad.tsyrklevich on Sep 15 2017, 1:25 PM.

Details

Summary

This is the follow-up patch to D37924.

This change refactors clang to use the the newly added section headers
in SpecialCaseList to specify which sanitizers blacklists entries
should apply to, like so:

[cfi-vcall]
fun:*bad_vcall*
[cfi-derived-cast|cfi-unrelated-cast]
fun:*bad_cast*

The SanitizerSpecialCaseList class has been added to allow querying by
SanitizerMask, and SanitizerBlacklist and its downstream users have been
updated to provide that information. Old blacklists not using sections
will continue to function identically since the blacklist entries will
be placed into a '[*]' section by default matching against all
sanitizers.

Diff Detail

Repository
rL LLVM

Event Timeline

Not included in this change:

  • Updating the sanitizer documentation to use section headers. It's worth doing for the CFI documentation to let people know they can break down blacklist entries by CFI mode, but is it worth pushing users to specify '[address]', '[memory]', etc. sections for the other sanitizers as well?
  • Merging the various sanitizer blacklists into a single blacklist. I'd like to pursue this in a later change where I also plan to break down the cfi blacklist by the specific CFI mode they should be blacklisted by.

Run clang-format.

eugenis added inline comments.Sep 15 2017, 4:51 PM
include/clang/Basic/SanitizerSpecialCaseList.h
33 ↗(On Diff #115509)

Please add a comment on the meaning of Mask. I assume it's any-of?

lib/CodeGen/CodeGenModule.cpp
1569 ↗(On Diff #115509)

would this blacklist [address] when compiling for kernel-address, and vice versa?

test/CodeGen/sanitizer-special-case-list.c
4 ↗(On Diff #115509)

it is common to add such files as actual files under Inputs/

include/clang/Basic/SanitizerSpecialCaseList.h
33 ↗(On Diff #115509)

Correct, I'll add a comment regarding this.

lib/CodeGen/CGDeclCXX.cpp
322 ↗(On Diff #115509)

This use of ASanMask could also confound address & kernel-address as @eugenis pointed out.

lib/CodeGen/CodeGenModule.cpp
1569 ↗(On Diff #115509)

Yes, it would. We should pass the actual sanitizer option mask ANDed with the ASanMask here. Thanks!

vlad.tsyrklevich marked 4 inline comments as done.Sep 15 2017, 7:33 PM

Update documentation to mention sections in the blacklist documentation and in the CFI documentation (where it's most useful)

Update for SectionsMap refactor in LLVM

eugenis added inline comments.Sep 18 2017, 5:31 PM
docs/SanitizerSpecialCaseList.rst
57 ↗(On Diff #115751)

Section names are regular expressions

also, not really a regular expression, more like a wildcard; I don't know what's the right term for this

lib/CodeGen/CGDeclCXX.cpp
322 ↗(On Diff #115509)

It would be more readable to split it in two if-s, IMHO: one for asan, one for kernel-asan.

  • Refactor to make compile() not virtual again
  • Refactor a confusing use of ASanMask into individual uses of SanitizerKind::{Address,KernelAddress}
  • 'Sections' -> 'Section names'
vlad.tsyrklevich marked an inline comment as done.Sep 19 2017, 9:05 AM
vlad.tsyrklevich added inline comments.
docs/SanitizerSpecialCaseList.rst
57 ↗(On Diff #115751)

They are not just wildcards in the sense that cfi-vcall|cfi-icall is more than just wildcard matching. I could add a line that explains how * behaves here as well? (similarly to what we have in the entries paragraph)

lib/CodeGen/CGDeclCXX.cpp
322 ↗(On Diff #115509)

Agreed.

eugenis added inline comments.
include/clang/Basic/SanitizerSpecialCaseList.h
39 ↗(On Diff #115849)

"compile" in this method name is confusing. It's used in the base class to refer to regexp compilation. Let's call this one createSanitizerSections.

lib/AST/Decl.cpp
3932 ↗(On Diff #115849)

"Asan" is a more common spelling in clang & compiler-rt identifiers (even though official abbreviation is ASan).

3953 ↗(On Diff #115849)

Looks like this is another case of missing "& LangOpts.Sanitize.Mask" ?

lib/Basic/XRayLists.cpp
29 ↗(On Diff #115849)

It feels redundant to have AlwaysInstrument and NeverInstrument lists, and then the same distinction in section names. Maybe sections could be named "xray" in both cases? Or, better, the lists could be merged into a single list with always and never sections? There is also an issue of backward compatibility. Anyway, that's for xray devs to decide. @dberris

lib/CodeGen/CodeGenModule.cpp
1564 ↗(On Diff #115849)

AsanMask

vlad.tsyrklevich marked 5 inline comments as done.
  • sanitizerCompile -> createSanitizerSections
  • ASanMask -> AsanMask and fix another bug relating to ANDing with LangOpts.Sanitize.Mask
lib/AST/Decl.cpp
3953 ↗(On Diff #115849)

Indeed.

lib/Basic/XRayLists.cpp
29 ↗(On Diff #115849)

I chose this approach for backwards compatibility, but I'd defer to what @dberris thinks is best.

dberris added inline comments.Sep 21 2017, 10:33 PM
lib/Basic/XRayLists.cpp
29 ↗(On Diff #115849)

Sorry for being late here.

I'm fine with keeping this as-is, then merging the lists into a single one. At the time this was designed/implemented, I hadn't thought about whether we could have used a single list. At the time it made sense to separate the always/never lists and applied in a set order (always wins over never).

If this is all-new functionality anyway, I'd think using "xray" as the section header and then using per-entry "always" and "never" identifiers/sections make sense.

If you leave a TODO here (and/or file a bug on XRay) I can do the migration to a single list later. I'm fine with how this is set as-is.

eugenis accepted this revision.Sep 22 2017, 2:25 PM
This revision is now accepted and ready to land.Sep 22 2017, 2:25 PM
This revision was automatically updated to reflect the committed changes.