[analyzer] Pointee of pointer-to-const is invalidated when there is a callback argument
- Dominant language
- LLVM
- Stars
- 40.5k
- Forks
- 18.7k
- PR merge metrics
- PR metrics pending
Description
The code of `CallEvent::invalidateRegions()` has logic to ensure that if the type of a function parameter is pointer-to-const, then the pointee of the corresponding argument is not invalidated. This is relevant in e.g. the following code fragment:
```c
void opaque(int *p, const int *q);
int simple(void) {
int x = 0, y = 0;
opaque(&x, &y);
int z = 10 / x; // no division by zero, x was invalidated
return z / y; // division by zero report, second parameter is pointer-to-const
}
```
This is implemented in a roundabout way: the indices of pointer-to-const parameters are collected in the set `PreserveArgs`, and then arguments whose indices appear in this set are marked with the invalidation trait `TK_PreserveContents`. (It would be easier to iterate over the arguments just once.)
However, the implementation also contains a cryptic `if (!argumentsMayEscape())` check (which was added in 2012) that disables this "don't invalidate pointees of pointer-to-const parameters" logic in the case when one of the argument is a non-null callback (function pointer or something like that). This causes a surprising false negative e.g. in the following code:
```c
void opaque_cb(int *p, const int *q, void (*cb)(void));
void cb(void);
int with_callback(void) {
int x = 0, y = 0;
opaque_cb(&x, &y, cb);
int z = 10 / x; // no division by zero, x was invalidated
return z / y; // false negative, y was also invalidated because there is a callback
}
```
This behavior does not make sense and – if I understand correctly – does not have any advantages. Somebody should carefully review the effects of the `if (!argumentsMayEscape())` check and if it is indeed useless here, then it should be removed. (After that, the code could be simplified by eliminating the pointless intermediate variable `PreserveArgs`.)
I found this false negative when I was reading the code of `CallEvent::invalidateRegions()` to understand https://github.com/llvm/llvm-project/issues/222102 (which is caused by _another_ independent deficiency of `invalidateRegions()`). My handwritten synthetic example triggers a false negative, but it would be also easy to craft inputs where this logic causes false positives.
[Godbolt reproducer](https://godbolt.org/#g:!((g:!((g:!((h:codeEditor,i:(filename:'1',fontScale:14,fontUsePx:'0',j:1,lang:___c,selection:(endColumn:15,endLineNumber:3,positionColumn:15,positionLineNumber:3,selectionStartColumn:15,selectionStartLineNumber:3,startColumn:15,startLineNumber:3),source:'void+opaque(int+*p,+const+int+*q)%3B%0Avoid+opaque_cb(int+*p,+const+int+*q,+void+(*cb)(void))%3B%0Avoid+cb(void)%3B%0A%0Aint+simple(void)+%7B%0A++++int+x+%3D+0,+y+%3D+0%3B%0A++++opaque(%26x,+%26y)%3B%0A++++int+z+%3D+10+/+x%3B+//+no+division+by+zero,+x+was+invalidated%0A++++return+z+/+y%3B+//+division+by+zero+report,+second+parameter+was+pointer-to-const%0A%7D%0Aint+with_callback(void)+%7B%0A++++int+x+%3D+0,+y+%3D+0%3B%0A++++opaque_cb(%26x,+%26y,+cb)%3B%0A++++int+z+%3D+10+/+x%3B+//+no+division+by+zero,+x+was+invalidated%0A++++return+z+/+y%3B+//+false+negative,+y+was+also+invalidated+because+there+is+a+callback%0A%7D%0Aint+with_null_callback(void)+%7B%0A++++int+x+%3D+0,+y+%3D+0%3B%0A++++opaque_cb(%26x,+%26y,+(void*)0)%3B%0A++++int+z+%3D+10+/+x%3B+//+no+division+by+zero,+x+was+invalidated%0A++++return+z+/+y%3B+//+division+by+zero+report,+null+callback+was+ignored%0A%7D'),l:'5',n:'0',o:'C+source+%231',t:'0')),k:41.734417344173444,l:'4',m:100,n:'0',o:'',s:0,t:'0'),(g:!((g:!((h:compiler,i:(compiler:cclang_trunk,filters:(b:'0',binary:'1',binaryObject:'1',commentOnly:'0',debugCalls:'1',demangle:'0',directives:'0',execute:'1',intel:'1',libraryCode:'0',trim:'1',verboseDemangling:'0'),flagsViewOpen:'1',fontScale:14,fontUsePx:'0',j:1,lang:___c,libs:!(),options:'--analyze',overrides:!(),selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1),l:'5',n:'0',o:'+x86-64+clang+(trunk)+(Editor+%231)',t:'0')),k:50,l:'4',m:50,n:'0',o:'',s:0,t:'0'),(g:!((h:output,i:(compilerName:'x86-64+gcc+16.1',editorid:1,fontScale:14,fontUsePx:'0',j:1,wrap:'1'),l:'5',n:'0',o:'Output+of+x86-64+clang+(trunk)+(Compiler+%231)',t:'0')),header:(),l:'4',m:50,n:'0',o:'',s:0,t:'0')),k:58.265582655826556,l:'3',n:'0',o:'',t:'0')),l:'2',n:'0',o:'',t:'0')),version:4) for those who want to play around. (This also demonstrates that if the function pointer argument is null, its presence does not affect anything.)
@benedekaibas or @tigbr This may be a fitting task for one of you if you're interested.
Contributor guide
Research direction
Start in CallEvent::invalidateRegions() and trace the effects of the !argumentsMayEscape() check on pointer-to-const arguments. Reproduce the simple, callback, and null-callback examples from the issue, then verify that callback presence no longer suppresses the expected division-by-zero report while null callbacks remain ignored. Done means the behavior is reviewed, simplified if appropriate, and covered by a suitable analyzer regression test.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c, cpp
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100