[PATCH] D30489: [analyzer] catch out of bounds for VLA

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D30489#689947, @zaks.anna wrote:

> Should we just remove ArrayBoundChecker.cpp or is there a value in keeping it around?


I think once ArrayBoundCheckerV2 is in production we should only keep ArrayBoundChecker there as educational purpuses for now. However, in the future, as the constraint manager and the core analyzer gets smarter, the V2 checker might get more and more similar to the original one. Once this patch is accepted, I might run the analyzer with V2 check enabled on some open source projects. It would be great to have it moved out of alpha soon.

> 
> 
>> If no, an alternative approach would be to properly set the constraints on the extent of the VLA's memory region.
> 
> For information I tried to set the extent for the VLA.. by copy/pasting some code from the other diff. Ideally I don't think it should be set in checkPreStmt but this was an experiment...
> 
>   void ArrayBoundCheckerV2::checkPreStmt(const ArraySubscriptExpr *A, CheckerContext &C) const
>   {
>     ASTContext &Ctx = C.getASTContext();
>     QualType T = A->getBase()->IgnoreCasts()->getType();
>     const VariableArrayType *VLA = Ctx.getAsVariableArrayType(T);
>     if (!VLA)
>       return;
>   
>     ProgramStateRef State = C.getState();
>   
>     SVal ElementCount = State->getSVal(VLA->getSizeExpr(), C.getLocationContext());
>     QualType ElementType = VLA->getElementType();
>     ASTContext &AstContext = C.getASTContext();
>     CharUnits TypeSize = AstContext.getTypeSizeInChars(ElementType);
>   
>     if (Optional<DefinedOrUnknownSVal> DefinedSize =
>       ElementCount.getAs<DefinedOrUnknownSVal>()) {
>       SVal AV = State->getSVal(A->getBase(), C.getLocationContext());
>       const SubRegion *Region = AV.getAsRegion()->getAs<SubRegion>();
>       SValBuilder &svalBuilder = C.getSValBuilder();
>       DefinedOrUnknownSVal Extent = Region->getExtent(svalBuilder);
>       // size in Bytes = ElementCount*TypeSize
>       SVal SizeInBytes = svalBuilder.evalBinOpNN(
>         State, BO_Mul, ElementCount.castAs<NonLoc>(),
>         svalBuilder.makeArrayIndex(TypeSize.getQuantity()),
>         svalBuilder.getArrayIndexType());
>       DefinedOrUnknownSVal extentMatchesSize = svalBuilder.evalEQ(
>         State, Extent, SizeInBytes.castAs<DefinedOrUnknownSVal>());
>       State = State->assume(extentMatchesSize, true);
>       C.addTransition(State);
>     }
>   }

You probably get an Unknown value for  `State->getSVal(VLA->getSizeExpr(), C.getLocationContext());`,  because the sizeExpr is not in the environment, it is already evaluated, its value has been used up, it is no longer needed. So in case the correct constraints are missing from the VLAs, you should add these assumptions at a point where this value is guaranteed to be available, probably right after the evaluation of the declaration.


Repository:
  rL LLVM

https://reviews.llvm.org/D30489