Page MenuHomePhabricator

[analyzer] ApiModeling: Add buffer size arg constraint
ClosedPublic

Authored by martong on Mar 30 2020, 8:53 AM.

Details

Summary

Introducing a new argument constraint to confine buffer sizes. It is typical in
C APIs that a parameter represents a buffer and another param holds the size of
the buffer (or the size of the data we want to handle from the buffer).

Diff Detail

Event Timeline

martong created this revision.Mar 30 2020, 8:53 AM

Please avoid to stuff in CheckerContext because this facility should be used by ExprEngine/Store as well.
Let us reword your API: getDynamicSizeWithOffset(ProgramStateRef, SVal, SValBuilder &). Of course we are trying to obtain some buffer-ish size, that is the purpose of the entire API.
I also could imagine something like getDynamicSizeMul(ProgramStateRef, const MemRegion &, const MemRegion &, SValBuilder &), as it is very common.

May it would make sense to use the API like:

getDynamicSizeWithOffset(State, MR, SVB) {
  Offset = State->getStoreManager().getStaticOffset(MR, SVB);
  ...
}

This idea is similar to MemRegionManager::getStaticSize(MR, SVB). Hopefully no-one needs dynamic offsets.

getDynamicSizeWithOffset(State, MR, SVB) {
  Offset = State->getStoreManager().getStaticOffset(MR, SVB);
  ...
}

Hm, the MemRegion's offset should be great. I was thinking about if we would store SVal offsets in the Store.

martong marked an inline comment as done.Apr 9 2020, 8:31 AM
martong added inline comments.
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
256

Use BinaryOperator::negateComparisonOp

martong updated this revision to Diff 257317.Apr 14 2020, 7:21 AM
martong marked an inline comment as done.
  • Rebase to master
  • Use BinaryOperator::negateComparisonOp
  • getBufferDynamicSize -> getDynamicSizeWithOffset

Please avoid to stuff in CheckerContext because this facility should be used by ExprEngine/Store as well.
Let us reword your API: getDynamicSizeWithOffset(ProgramStateRef, SVal, SValBuilder &). Of course we are trying to obtain some buffer-ish size, that is the purpose of the entire API.
I also could imagine something like getDynamicSizeMul(ProgramStateRef, const MemRegion &, const MemRegion &, SValBuilder &), as it is very common.

May it would make sense to use the API like:

getDynamicSizeWithOffset(State, MR, SVB) {
  Offset = State->getStoreManager().getStaticOffset(MR, SVB);
  ...
}

This idea is similar to MemRegionManager::getStaticSize(MR, SVB). Hopefully no-one needs dynamic offsets.

Thanks for the review! I've update the API as you suggested.

martong updated this revision to Diff 262095.May 5 2020, 6:39 AM
  • Rebase to master

I'm not familiar enough with DynamicSize.cpp to judge the changes there, but aside from a few nits, this LGTM.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
249–250

Would an unreachable be appropriate here then?

993

In most places, where we refer to an argument number, we use ArgNo. Is there a reason we don't do that here? Can we enforce greater type safety?

martong updated this revision to Diff 266287.EditedMay 26 2020, 11:59 AM
martong marked 4 inline comments as done.
  • REBASE to master
  • Add assert
  • Create the BufferSize arg constraints in a more readable way

The rebase to master makes a bit hard to see the exact changes in this update. I should have done the rebase in a separate update, sorry about that.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
249–250

Yes, good point, just added that. CallAndMessage is already a dependency, so this must not fire.

993

Yeah, good point, I am going with this:

BufferSize(/*Buffer=*/ArgNo(0), /*BufSize=*/ArgNo(1))));

About type safety: I was thinking about a strong typedef, but we don't actually have a convenient template for that in LLVM. And most of the time here in LLVM people just apply the /*Arg=*/ pythonish form.

Szelethus accepted this revision.May 29 2020, 5:02 AM
Szelethus added a subscriber: balazske.

LGTM! I think you could remove the extra parameter, but I don't think this warrants another round of review on my end. However, the dynamic size change seems very unfamiliar to me. @baloghadamsoftware, @balazske, could you take a peek?

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
237

You can retrieve SValBuilder from ProgramState: State->getStateManager().getSValBuilder().

This revision is now accepted and ready to land.May 29 2020, 5:02 AM
martong updated this revision to Diff 267216.May 29 2020, 6:20 AM
martong marked 2 inline comments as done.
  • Remove SValBuilder param
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
237

Thanks!

This revision was automatically updated to reflect the committed changes.
  • Remove SValBuilder param

I failed to emphasize that the point was to remove the new CheckerContext parameters :)