Page MenuHomePhabricator

[analyzer] DynamicSize: Store the dynamic size
Needs ReviewPublic

Authored by Charusso on Nov 1 2019, 11:57 AM.

Details

Reviewers
NoQ
Summary

This patch introduces a way to store the symbolic size.

Diff Detail

Event Timeline

Charusso created this revision.Nov 1 2019, 11:57 AM
Charusso marked an inline comment as done.Nov 1 2019, 12:06 PM

I do not want to reverse engineer the MallocChecker to obtain the size on call basis. The current model without D68725 makes it enough difficult to obtain the size even with this generic map, but it is working fine.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
1073 ↗(On Diff #227500)

That dual assumption made changes in the test files, and there is no other dual assumption.

NoQ added inline comments.Nov 1 2019, 1:12 PM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
29–30

Why do we need this?

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
1073 ↗(On Diff #227500)

Wait, this code should not have been changed. It's not an allocation site, we don't receive any new information about the size of the region here.

clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
180

This gets rid of the assertion failure in https://bugs.llvm.org/show_bug.cgi?id=28450 by implementing my suggestion (2). Yay.

Charusso updated this revision to Diff 227534.Nov 1 2019, 2:06 PM
Charusso marked 6 inline comments as done.
  • Fix.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
29–30

I think as the checkers are growing and we push more-and-more allocation modeling so that at some point the Git's 8-parameter allocator's size expression could be retrieved so easily. This is the full arsenal of my buffer-overflow checker's needs, so I have pushed it here. Also it made a meaning to have a helper-class with two fields (one would be lame).

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1414

My problem was only that. It partially repeats the ExprEngine::bindReturnValue(), which is a wonky design. I will look into that later.

clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
180

Cool!

Changes to MallocChecker really highlight the positive effects of this patch. Nice!

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
451

Is it possible to merge these parameters into a single DynamicSizeInfo object?

1167

and here?

Charusso marked 3 inline comments as done.Nov 4 2019, 8:20 AM

Changes to MallocChecker really highlight the positive effects of this patch. Nice!

Thanks!

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
451

I want to do the opposite so I want to hide such constructors with the global getters and setters to make it easier to use. It turns out you only want to obtain one of its fields at a time, so that it is a bad idea to give you the entire object with multiple metadata. In my mind they will be in a dynamic namespace, and we could write that: dynamic::getSize(State, MR) or dynamic::getType(State, MR).

I like the idea of creating Contexts to store metadata, but this is not the use case of the dynamic information, I think. The Dynamic*Info is good for being stored in a map, and other than that it is just boilerplate to obtain the object and call one of its fields which I do not like anymore.

NoQ added inline comments.Nov 4 2019, 11:54 AM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
29–30
In D68725#1722136, @NoQ wrote:

any path-sensitive checker for which such region is "interesting" would have to implement a bug visitor to display the allocation site. Such visitor automatically knows the location of the alloca() expression and can emit the necessary fixits.

Again, you will have to highlight the allocation site with a note. Therefore you will have to write a bug visitor that traverses the size expression at some point (or, equivalently, a note tag when the size expression is evaluated). Therefore you don't need to store the expression in the program state.

Charusso abandoned this revision.Thu, Jan 30, 7:57 AM
Charusso marked an inline comment as done.

Let us say, we do not need this patch. In case we would need it, I will reopen.

Charusso updated this revision to Diff 241749.EditedFri, Jan 31, 9:10 AM
Charusso edited the summary of this revision. (Show Details)
  • Let us reuse this patch.
  • Remove the expression storing feature.
  • Only store known sizes.
NoQ added a comment.Thu, Feb 20, 2:02 PM

Hmm, has this patch not landed yet? I was so excited to finally have https://bugs.llvm.org/show_bug.cgi?id=28450 fixed :)

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
767

This looks like an object count, we'll need to convert it to size in bytes because that's what extent is.

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
698

Same. I guess we should add a test for both cases, given that nothing failed when we screwed up the extent.