Page MenuHomePhabricator

Implement no_sanitize_address for global vars
Needs ReviewPublic

Authored by dougk on Nov 9 2016, 8:44 AM.

Details

Summary

This was already submitted as r284272.

Regarding the use of Attr as a local variable name, I would prefer to remain consistent with the existing code in CodeGenFunction.cpp which was not touched by this patch.

line 720: for (auto Attr : D->specific_attrs<NoSanitizeAttr>())

but I'm certainly not opposed to changing both places.

Diff Detail

Event Timeline

dougk updated this revision to Diff 77357.Nov 9 2016, 8:44 AM
dougk retitled this revision from to Implement no_sanitize_address for global vars.
dougk updated this object.
dougk added a reviewer: aaron.ballman.
dougk added a subscriber: cfe-commits.
kcc added a reviewer: eugenis.Nov 9 2016, 11:23 AM
dougk added a comment.Nov 9 2016, 12:07 PM

Also note: In this change, the diagnostic messages are correct; it's the enum name that's bad. The diagnostic message is wrong for the 'section' attribute, which is fixed by https://reviews.llvm.org/D26459

aaron.ballman added inline comments.Nov 10 2016, 6:56 AM
lib/CodeGen/SanitizerMetadata.cpp
68

Should use const auto * (feel free to change the other use(s) in a separate commit, no review required).

lib/Sema/SemaDeclAttr.cpp
5316

This formatting change is unrelated.

5343

You diagnose this as an error, but don't early return if the attribute is invalid. Is that intentional?

5364

You diagnose it as an error, but then add the attribute anyway. Is that intentional?

test/CodeGen/asan-globals.cpp
10

You modified no_sanitize_address, but did not add any test cases for it.

test/SemaCXX/attr-no-sanitize-address.cpp
24

Please add a new test case to replace this one, showing that the attribute is properly diagnosed when applied to something the attribute cannot appertain to.

test/SemaCXX/attr-no-sanitize.cpp
5 ↗(On Diff #77357)

Please add a new test case to replace this one, showing that the attribute is properly diagnosed when applied to something the attribute cannot appertain to.

dougk updated this revision to Diff 77658.Nov 11 2016, 12:43 PM
dougk marked 2 inline comments as done.

changes per Aaron Ballman

kcc added a subscriber: kcc.Nov 11 2016, 5:34 PM

Does this change deserve a documentation update?

dougk updated this revision to Diff 77815.Nov 14 2016, 8:12 AM
dougk marked an inline comment as done.

add a sentence about the change in AddressSanitizer.rst

aaron.ballman added inline comments.Nov 15 2016, 10:36 AM
lib/Sema/SemaDeclAttr.cpp
5348

This is unfortunate because the declarative subject information from Attr.td does not match the semantics in SemaDeclAttr.cpp for many of the sanitizer names. For instance, this means that someone using __attribute__((no_sanitize("thread"))` on a static local variable will get a diagnostic telling them that it only applies to functions, methods, and global variables. If they make their static local variable into a global variable, they will then get a different diagnostic telling them, oops, just functions or methods.

While I understand that this was already submitted in r284272, I don't see any rationale given in the commit log as to why this behavior is desirable in the first place. Can you give me some background information on why this is needed? Does it apply to other sanitizer names as well?

Suppression of sanitizing is necessary if the variable is magically a memory-mapped device I/O address.
The linker can arrange for this to be the case using fancy scripts, or even just as simple as a section attribute that requires that you take up exactly a certain number of bytes in the section.
There was some thought that any non-default section should preclude sanitization, but Kostya said that, no, it would make sense to require explicit no-sanitize. I (mistakenly) took that to mean "just do it", for which I apologize.

lib/Sema/SemaDeclAttr.cpp
5316

reverted. (clang-format-diff did that on account of proximity to the added lines)

5343

not intentional. fixed

5364

not intentional. fixed

test/SemaCXX/attr-no-sanitize-address.cpp
24

that's already tested by noanal_testfn which has no_sanitize_address on 'int x = y';

aaron.ballman edited edge metadata.Dec 19 2016, 8:31 AM

Suppression of sanitizing is necessary if the variable is magically a memory-mapped device I/O address.
The linker can arrange for this to be the case using fancy scripts, or even just as simple as a section attribute that requires that you take up exactly a certain number of bytes in the section.
There was some thought that any non-default section should preclude sanitization, but Kostya said that, no, it would make sense to require explicit no-sanitize. I (mistakenly) took that to mean "just do it", for which I apologize.

Thank you for the explanation, but I'm still wondering if this applies to only address sanitization, or to others as well, such as memory or thread. The fact that the declarative information in Attr.td is incorrect now is a concern. We can either address that concern by making the conflict go away for all sanitizer strings (which may not be sensible), or by making some complex changes to the way subject lists work (which may be more work than it's worth). The point to having a single no_sanitize attribute was so that we didn't need to add a new attribute every time we came up with a new sanitizer, but it was understood that this works because all of the sanitizers apply to the same constructs. This change breaks that assumption so that now one of the sanitizer strings diverges from all the rest, and I would like to avoid that.

dougk added a comment.Dec 19 2016, 9:32 AM

I think it probably works to have the attribute appertain to any sanitizer. I did not know that it did, so I conservatively assumed that it didn't. I'll go ahead and make things consistent, and confirm that it dtrt.