[PATCH] D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D158707#4614268 <https://reviews.llvm.org/D158707#4614268>, @danix800 wrote:

> In D158707#4614100 <https://reviews.llvm.org/D158707#4614100>, @steakhal wrote:
>
>> As a general comment on requiring all extents to be of unsigned APSInts.
>> Checkers, like the `MallocChecker`, binds the result of arbitrary expression's values (the argument of malloc, for example) as extents of some regions.
>> Usually, that argument is of an `unsigned` static type, thus even if some other expression is there, and implicit cast will turn it into `unsigned`.
>> However, in CSA, we don't respect such casts, thus if it was signed, we will bind a signed value as an extent.
>>
>> Here is an example <https://godbolt.org/z/jv5e8EoaM> demonstrating this:
>>
>>   int n = conjure();
>>   clang_analyzer_dump(n);  // conj
>>   clang_analyzer_value(n); // i32
>>   
>>   char *p = (char*)malloc(n);
>>   clang_analyzer_dump(clang_analyzer_getExtent(p));  // conj
>>   clang_analyzer_value(clang_analyzer_getExtent(p)); // i32
>>
>> Consequently, I'm not sure it's possible to turn extents "unsigned", without fixing casts.
>
> Wow! Really another unexpectation to me!
>
> I think either signedness is ok, the stored data would not be lost. How clients use these values are the actual problem.

I'm not sure how could the clients, aka. checkers use extents "correctly". They cannot even explicitly wrap the extent by an "unsigned" type e.g. by modeling a cast to "size_t" because that is a noop, thus adding casts is an identity operation. See here <https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp#L976>.
Consequently, checkers using extents in comparisons or doing arithmetic in the symbolic domain e.g. when doing bounds checks under the hood, will be affected by the missing cast there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158707