Page MenuHomePhabricator

[analyzer] Avoid checking addrspace pointers in cstring checker
ClosedPublic

Authored by vabridgers on Jan 24 2022, 9:05 AM.

Details

Summary

This change fixes an assert that occurs in the SMT layer when refuting a
finding that uses pointers of two different sizes. This was found in a
downstream build that supports two different pointer sizes, The CString
Checker was attempting to compute an overlap for the 'to' and 'from'
pointers, where the pointers were of different sizes.

In the downstream case where this was found, a specialized memcpy
routine patterned after memcpy_special is used. The analyzer core hits
on this builtin because it matches the 'memcpy' portion of that builtin.
This cannot be duplicated in the upstream test since there are no
specialized builtins that match that pattern, but the case does
reproduce in the accompanying LIT test case. The amdgcn target was used
for this reproducer. See the documentation for AMDGPU address spaces here
https://llvm.org/docs/AMDGPUUsage.html#address-spaces.

The assert seen is:

`*Solver->getSort(LHS) == *Solver->getSort(RHS) && "AST's must have the same sort!"'

Ack to steakhal for reviewing the fix, and creating the test case.

Diff Detail

Event Timeline

vabridgers created this revision.Jan 24 2022, 9:05 AM
vabridgers requested review of this revision.Jan 24 2022, 9:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2022, 9:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steakhal added a subscriber: NoQ.Jan 25 2022, 1:49 AM

I'm wondering if we should have an assertion within the SValBuilder::evalBinOpLL() asserting that the pointers should have the same bitwidths.
That's better than having nothing, waiting for a bugreport containing such a constraint on the bugpath to get a crash with Z3 refutation.

That way we could have a chance to fix these issues for once and all.
WDYT @NoQ @martong?

clang/test/Analysis/cstring-checker-addressspace.c
14

Please try to trigger the test using some suffix for the memcpy.
E.g.: memcpy_host2device or something similar.

Good thoughts, I will try those things and get back to you.

  • update test case
  • add initial Loc bitwidth check for evalBinOpLL for review
vabridgers edited the summary of this revision. (Show Details)Jan 26 2022, 8:11 AM

I updated the test case after verifying I can reproduce the crash without the fix. I also experimented with adding a bitwidth check for the lhs and rhs loc parameters to evalBinOpLL, and verified this assert catches the negative (crash) case.

I explored adding a 'memcpy_special' case, but this memcpy function must be a builtin for the checker to hit on it, so I could not repro.

I realize the bitwidth check may not be ideal, but it's a start. Thanks in advance for comments. Best!

vabridgers marked an inline comment as done.Jan 26 2022, 8:16 AM
vabridgers added inline comments.
clang/test/Analysis/cstring-checker-addressspace.c
14

Please see latest comment. I tried this, but found a memcpy name with some suffix must be builtin. I could not find a way to repro this, but I was able to repro the negative case with the changes I just uploaded amending this review.

steakhal added inline comments.Jan 28 2022, 1:40 AM
clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
718 ↗(On Diff #403276)

Why don't you simply call loc->getType() end delegate the heavy lifting to that?
If you have a meaningful type, you could acquire the size of that using the ASTContext exactly how you did.

After doing so, this would reduce the complexity of checking this, so the assertion could be placed into the body of evalBinOpLL().

723–725 ↗(On Diff #403276)

Remove this default case. Simply return 0; from the end of the function.
Also, I would recommend return-ing from the handled cases instead of mutating a variable.

738 ↗(On Diff #403276)

What if Loc is a void*. What would be the size of that one?

746 ↗(On Diff #403276)

assertEqualBitwidths would be probably more appropriate - even if this is a best-effort assertion.

766 ↗(On Diff #403276)

Please have something like this in the assertion. That way we could immediately see why this is an issue.

clang/test/Analysis/cstring-checker-addressspace.c
18

Remove this line, it's redundant.

NoQ added inline comments.Jan 28 2022, 11:29 AM
clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
728 ↗(On Diff #403276)

That's a pretty big "if".

The natural way to handle the opposite case is TypedRegion::getLocationType().

You can probably go with SVal::getType() here as it happens to do the right thing (treat Loc values as if they were pointers). But, again, it's speculative.

750–751 ↗(On Diff #403276)

I think this is a good, correct assertion. It is up to the AST to insert implicit casts when bit widths don't match (or outright reject the code, I've no idea what really happens in such cases, but it's up to the compiler in any case).

vabridgers marked an inline comment as done.Jan 29 2022, 5:42 AM

Thanks for the comments, I'll address. Best

I took a run at the improvements, and the rabbit hole keeps getting deeper :/ @steakhal, I'll contact you offline to work out the problems. Best

vabridgers updated this revision to Diff 404940.Feb 1 2022, 7:58 AM

Simplify assertion per comments.
Add a "temp" fix in SValBuilder that permits the additional
test cases to pass without crash

I think @steakhal will have a more comprehensive change to back out the makeNull() calls to makeNullWithWidth() calls. I'll back out the change to SValBuilder.cpp once that change is pushed. For now, this is my update. Thanks

vabridgers marked 7 inline comments as done.Feb 1 2022, 8:02 AM

I think the comments have been addressed for now. Please let me know if I missed something, or anything else needs to be done (besides back out the change to SValBuilder.cpp when ready). Thanks for the comments. Best

Update after makeNull fixes

steakhal accepted this revision.Feb 22 2022, 5:53 AM

Besides a couple of nits, it's ready for landing.
Thanks for the hard work @vabridgers!

clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
726–727 ↗(On Diff #408104)

You should probably ignore references.
Ah, but we already lost that information. Well, that's bad.
Anyway. You probably don't need to check the subkinds. Do you?

745 ↗(On Diff #408104)

You don't use the return value of this call.
Can we have a void return type instead?

This revision is now accepted and ready to land.Feb 22 2022, 5:53 AM
vabridgers added inline comments.Feb 24 2022, 1:09 AM
clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
726–727 ↗(On Diff #408104)

There was a case I hit in my testing where the subkinds were different and a crash occurred. I cannot recall the details at the moment, but it made sense at the time to also check subkkinds. I'll check this again so I can describe the rationale for this.

745 ↗(On Diff #408104)

Yes, will do.

rebase, check ci

Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2022, 4:41 PM

add NDEBUG around assert

I added #ifndef NDEBUG around the assert since I'm not sure if downstream consumers that use different pointer sizes by address space will hit this assert. Our downstream target causes this assert to fire, so I will be continuing to find and fix these issues, submitting test cases as I can. Heck, we may even have to submit a mock target to properly suss these out :)

Thanks for the support for sorting these out. Best

vabridgers planned changes to this revision.Mar 31 2022, 7:39 AM

I need to make a few changes. Please hold off any comments until the next patch, coming soon.

make assert function void, update summary

This revision is now accepted and ready to land.Mar 31 2022, 8:05 AM
vabridgers retitled this revision from [analyzer] Different address spaces cannot overlap to [analyzer] Avoid checking addrspace pointers in cstring checker.Mar 31 2022, 8:06 AM
steakhal accepted this revision.Mar 31 2022, 8:10 AM

Looks great!

This revision was landed with ongoing or failed builds.Mar 31 2022, 8:35 AM
This revision was automatically updated to reflect the committed changes.