Page MenuHomePhabricator

[hwasan] also omit safe mem[cpy|mov|set].
ClosedPublic

Authored by fmayer on Sep 15 2021, 2:14 AM.

Diff Detail

Event Timeline

fmayer created this revision.Sep 15 2021, 2:14 AM
fmayer updated this revision to Diff 372665.Sep 15 2021, 2:28 AM

more test

fmayer updated this revision to Diff 372686.Sep 15 2021, 6:19 AM

fix test

fmayer published this revision for review.Sep 15 2021, 6:35 AM
fmayer added a reviewer: eugenis.
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2021, 6:35 AM

This leads to a 20 % runtime improvement on PDFium, when stack instrumentation is disabled.

eugenis added inline comments.Sep 15 2021, 12:37 PM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
806–807

Is this a separate bugfix?

Am I right that this is not needed for regular load/store because the argument is required to be 100% traceable to a single alloca, but 2-args memintrinsics are safe if one arg is 100%, and the other is 100% not stack? That does not sound right.

The comment on accessIsSafe does not even try to cover such cases. Also, I do not see any tests under Analysis/StackSafety for the mixed memintrinsic case.

llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll
132

If lifetimes are irrelevant to a test case, you can just remove them altogether.

eugenis added inline comments.Sep 15 2021, 4:06 PM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
1000

Consider getDest / getSource for readability.

fmayer updated this revision to Diff 372902.Sep 16 2021, 5:04 AM
fmayer marked an inline comment as done.

use getDest/getSource

fmayer added inline comments.Sep 16 2021, 5:05 AM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
806–807

Before this change, we only checked this for instructions that took a single pointer variable. There were two cases in accessIsSafe:

  • the pointer is non stack: we never reached this instruction in the stack safety pass, and we return false because we do not find it in the mapping.
  • the pointer is potentially stack: we reach this instruction in the pass, and SECV will take care of checking whether we can *only* reach it from the alloca.

Now, for memcpy / memmove there is the extra case that we can have both cases for two arguments. Now the problem becomes if one argument is in range of the alloca, but the other one isn't reachable from any alloca, accessIsSafe will return true.

What I did in this change is clarify the semantics (as they already were) of accessIsSafe in the comment, and defer responsibility to check whether *all* arguments are potentially stack to the caller. As such, we only hit the second case above.

llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll
132

The stack safety does look at lifetimes, so to make it more representative I put them there. Either way is fine, WDYT?

eugenis added inline comments.Sep 16 2021, 1:44 PM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
806–807

Yeah, I agree with that. I just want it clarified even more.

For a sample of cases:

  1. dest and source both are always stack and safe
  2. dest is always stack and safe; source is never stack
  3. dest is either stack and safe or not stack; source is never stack

the comment does not give me confidence in what isAccessSafe answer would be.

llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll
132

There are no lifetimes at -O0, for example. It is generally a good idea to focus each test case on a single concern - if you are testing hwasan handling of memmove() with one safe and one unsafe argument, lifetimes are irrelevant and only hurt test readability.

It is good to have some tests with lifetimes, but they don't really need to be present *everywhere*.

fmayer updated this revision to Diff 373528.Sep 20 2021, 2:49 AM

more tests & comments

fmayer marked 4 inline comments as done.Sep 20 2021, 2:51 AM
fmayer added inline comments.
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
806–807

I clarified the comment and added some more tests.

llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll
132

Sure, removed for legibility (though with sanitizers enabled, there are always lifetimes, and this is a sanitizer test).

eugenis added inline comments.
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
806–807

I still find the description confusing, especially around "potential stack accesses".

How about this:

  • rename to stackAccessIsSafe
  • describe approx. like this: "returns true if the instruction can be proven to do only two types of memory accesses (1) live stack locations in-bounds or (2) non-stack locations"

i.e. not "accesses that may go to stack are safe" but "safe when goes to stack".
And this makes it clear that predicating the check with findAllocaForValue() makes it indeed 100% safe.

To be true to this description, the function should return true if the argument can not be traced back to an alloca. I think this can be implemented by removing "SafeAccesses" and looking straight at "AccessIsUnsafe".

WDYT?
@vitalybuka

806–807

Now since both branches do interesting stuff only when findAllocaForValue is non-null, we can hoist this predicate.

if (findAllocaForValue(Ptr)) {
  if (!InstrumentedStack) return true;
  if ((SSI && SSI->accessIsSafe(*Inst)) return true;
}
fmayer updated this revision to Diff 373820.Sep 21 2021, 2:33 AM
fmayer marked 2 inline comments as done.

address comment

fmayer marked an inline comment as done.Sep 21 2021, 2:34 AM
fmayer updated this revision to Diff 373878.Sep 21 2021, 5:40 AM

match logic to comment

Sorry, still need to sort out the other stack safety IR tests.

fmayer updated this revision to Diff 373926.Sep 21 2021, 7:34 AM

fix tests

Sorry, still need to sort out the other stack safety IR tests.

done.

I remember from the my C days that one of gotchas with memmove/memcpy is overlapping regions. Not sure if it is relevant the context of this change, just wanted to mention it.

llvm/include/llvm/Analysis/StackSafetyAnalysis.h
82

I remember from the my C days that one of gotchas with memmove/memcpy is overlapping regions. Not sure if it is relevant the context of this change, just wanted to mention it.

Thanks! That should be okay, we look at the two parameters separately; there is even a test that has memmove to itself (@test_in_range4).

eugenis accepted this revision.Sep 21 2021, 10:22 AM

LGTM

llvm/lib/Analysis/StackSafetyAnalysis.cpp
928

This is kind of ugly, but ok for a debug print. We could also

  • skip the instructions that are not in AccessIsUnsafe
  • print unsafe instructions instead of safe ones.

Up to your what seems easiest.

This revision is now accepted and ready to land.Sep 21 2021, 10:22 AM
fmayer marked 2 inline comments as done.Sep 22 2021, 2:31 AM
fmayer added inline comments.
llvm/lib/Analysis/StackSafetyAnalysis.cpp
928

Makes sense. Let's submit this and maybe do this in a follow up.

fmayer updated this revision to Diff 374170.Sep 22 2021, 3:07 AM
fmayer marked an inline comment as done.

resolve test confusion

This revision was landed with ongoing or failed builds.Sep 22 2021, 3:09 AM
This revision was automatically updated to reflect the committed changes.