[clang] [analyzer] Fix security.VAList false positives with C23 va_start (PR #192024)

[Donát Nagy via cfe-commits]

================
@@ -136,13 +136,14 @@ const SmallVector<VAListChecker::VAListAccepter, 15>
 const CallDescription VAListChecker::VaStart(CDM::CLibrary,
                                              {"__builtin_va_start"}, /*Args=*/2,
                                              /*Params=*/1),
+    VAListChecker::VaStartC23(CDM::CLibrary, {"__builtin_c23_va_start"}),
----------------
NagyDonat wrote:

I realized that this PR may expose a sneaky corner case.

Notice that the call description for `__builtin_va_start` only matches calls where two arguments are specified (and the declaration has one parameter – don't ask why this clang builtin claims to have one parameter, but apparently it works). This pedantry ensures that `checkVAListStartCall` can rely on the presence of two arguments. [1]

For `VAStartC23` you were right to omit the `Args` field (i.e. allow matching any argument count) because you need to match `__builtin_c23_va_start` calls with 1, 2 and perhaps more arguments. However, this way if the analyzer encounters the grossly invalid expression `__builtin_c23_va_start()` (call with no arguments), then `checkVAListStartCall` will receive a call with no arguments, and there I fear that calls like `Call.getArgSVal(0)` (= get the value of argument index 0) will cause an analyzer crash / assertion failure.

E.g. on current trunk the analyzer can parse [this degenerate source file](https://godbolt.org/#g:!((g:!((g:!((h:codeEditor,i:(filename:'1',fontScale:17,fontUsePx:'0',j:1,lang:___c,selection:(endColumn:1,endLineNumber:5,positionColumn:1,positionLineNumber:5,selectionStartColumn:1,selectionStartLineNumber:5,startColumn:1,startLineNumber:5),source:'/*+Type+your+code+here,+or+load+an+example.+*/%0Aint+foo(void)+%7B%0A++++__builtin_c23_va_start()%3B%0A%7D%0A'),l:'5',n:'0',o:'C+source+%231',t:'0')),k:48.76414273281115,l:'4',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:'0',libraryCode:'1',trim:'0',verboseDemangling:'0'),flagsViewOpen:'1',fontScale:14,fontUsePx:'0',j:1,lang:___c,libs:!(),options:'--analyze+-Wno-implicit-function-declaration',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:46.688710992440406,l:'4',m:45.03774363106201,n:'0',o:'',s:0,t:'0'),(g:!((h:output,i:(compilerName:'x86-64+clang+19.1.0+(assertions)',editorid:1,fontScale:14,fontUsePx:'0',j:1,wrap:'0'),l:'5',n:'0',o:'Output+of+x86-64+clang+(trunk)+(Compiler+%231)',t:'0')),header:(),l:'4',m:54.962256368938,n:'0',o:'',s:0,t:'0')),k:51.23585726718886,l:'3',n:'0',o:'',t:'0')),l:'2',m:100,n:'0',o:'',t:'0')),version:4) (in non-C23 mode if `-Wno-implicit-function-declaration` suppresses an error), but with your patch I suspect (although I'm not 100% sure) that this would crash the analyzer.

To resolve this (mostly theoretical) issue, I would suggest adding
```c++
  if (Call.getNumArgs() == 0)
    return; // Prevent a crash on grossly invalid code.
```
at the beginning of `checkVAListStartCall`.

After doing this, you can remove the `/*Args=*/ 2` and `/*Params=*/ 1` fields of `VaStart` because they won't be relevant anymore (and the difference between them may confuse the readers).

--------

[1] This is a common pattern: instead of writing ad hoc `if`s, the argument count validation is traditionally specified in call description matching. This is especially relevant for other checkers that match functions whose name may reasonably collide with unrelated user-defined functions.


https://github.com/llvm/llvm-project/pull/192024