[PATCH] D109816: [hwasan] also omit safe mem[cpy|mov|set].

[Florian Mayer via Phabricator via llvm-commits]

fmayer marked 4 inline comments as done.
fmayer added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:789
       return true;
-  } else if (SSI && SSI->accessIsSafe(*Inst)) {
+  } else if (SSI && SSI->accessIsSafe(*Inst) && findAllocaForValue(Ptr)) {
     return true;
----------------
eugenis wrote:
> fmayer wrote:
> > eugenis wrote:
> > > 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.
> > 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.
> 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.
> 
I clarified the comment and added some more tests.


================
Comment at: llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll:132
+  %y = bitcast [10 x i8]* %buf.sroa.1 to i8*
+  call void @llvm.lifetime.start.p0i8(i64 10, i8* nonnull %x)
+  call void @llvm.lifetime.start.p0i8(i64 10, i8* nonnull %y)
----------------
eugenis wrote:
> fmayer wrote:
> > eugenis wrote:
> > > If lifetimes are irrelevant to a test case, you can just remove them altogether.
> > > 
> > The stack safety does look at lifetimes, so to make it more representative I put them there. Either way is fine, WDYT?
> 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*.
> 
Sure, removed for legibility (though with sanitizers enabled, there are always lifetimes, and this is a sanitizer test).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109816/new/

https://reviews.llvm.org/D109816