llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Björn Svensson (bjosv) <details> <summary>Changes</summary> The `security.VAList` checker only recognized `__builtin_va_start` when matching `va_start` calls. In C23, `va_start` was changed to expand to `__builtin_c23_va_start` instead, causing the checker to never see the initialization. This resulted in false positives for every use of `va_arg`, `va_end`, `va_copy`, and functions like `vsnprintf` on any `va_list` initialized with the C23 `va_start`. Add a CallDescription for `__builtin_c23_va_start` and match it alongside the existing `__builtin_va_start`. --- Full diff: https://github.com/llvm/llvm-project/pull/192024.diff 5 Files Affected: - (modified) clang/docs/ReleaseNotes.rst (+6) - (modified) clang/lib/StaticAnalyzer/Checkers/VAListChecker.cpp (+3-2) - (added) clang/test/Analysis/Inputs/system-header-simulator-for-valist-c23.h (+16) - (added) clang/test/Analysis/valist-uninitialized-c23.c (+61) - (added) clang/test/Analysis/valist-unterminated-c23.c (+35) ``````````diff diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 3e2d287d1eb1f..019e01fda378f 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -611,6 +611,12 @@ Code Completion Static Analyzer --------------- +Crash and bug fixes +^^^^^^^^^^^^^^^^^^^ + +- Fixed ``security.VAList`` checker producing false positives when analyzing + C23 code where ``va_start`` expands to ``__builtin_c23_va_start``. + .. comment: This is for the Static Analyzer. Using the caret `^^^` underlining for subsections: diff --git a/clang/lib/StaticAnalyzer/Checkers/VAListChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/VAListChecker.cpp index e81c8bfa94cb1..bc8c82100b6fe 100644 --- a/clang/lib/StaticAnalyzer/Checkers/VAListChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/VAListChecker.cpp @@ -64,7 +64,7 @@ class VAListChecker : public Checker<check::PreCall, check::PreStmt<VAArgExpr>, int ParamIndex; }; static const SmallVector<VAListAccepter, 15> VAListAccepters; - static const CallDescription VaStart, VaEnd, VaCopy; + static const CallDescription VaStart, VaStartC23, VaEnd, VaCopy; public: void checkPreStmt(const VAArgExpr *VAA, CheckerContext &C) const; @@ -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"}), VAListChecker::VaCopy(CDM::CLibrary, {"__builtin_va_copy"}, 2), VAListChecker::VaEnd(CDM::CLibrary, {"__builtin_va_end"}, 1); } // end anonymous namespace void VAListChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { - if (VaStart.matches(Call)) + if (VaStart.matches(Call) || VaStartC23.matches(Call)) checkVAListStartCall(Call, C); else if (VaCopy.matches(Call)) checkVAListCopyCall(Call, C); diff --git a/clang/test/Analysis/Inputs/system-header-simulator-for-valist-c23.h b/clang/test/Analysis/Inputs/system-header-simulator-for-valist-c23.h new file mode 100644 index 0000000000000..89bc95abdad5c --- /dev/null +++ b/clang/test/Analysis/Inputs/system-header-simulator-for-valist-c23.h @@ -0,0 +1,16 @@ +// Like the compiler, the static analyzer treats some functions differently if +// they come from a system header -- for example, it is assumed that system +// functions do not arbitrarily free() their parameters, and that some bugs +// found in system headers cannot be fixed by the user and should be +// suppressed. + +#pragma clang system_header + +typedef __builtin_va_list va_list; + +#define va_start(...) __builtin_c23_va_start(__VA_ARGS__) +#define va_end(ap) __builtin_va_end(ap) +#define va_arg(ap, type) __builtin_va_arg(ap, type) +#define va_copy(dst, src) __builtin_va_copy(dst, src) + +int vprintf(const char *restrict format, va_list arg); diff --git a/clang/test/Analysis/valist-uninitialized-c23.c b/clang/test/Analysis/valist-uninitialized-c23.c new file mode 100644 index 0000000000000..9ec99ffe3c74b --- /dev/null +++ b/clang/test/Analysis/valist-uninitialized-c23.c @@ -0,0 +1,61 @@ +// RUN: %clang_analyze_cc1 -triple hexagon-unknown-linux -std=c23 -verify %s \ +// RUN: -analyzer-checker=core,security.VAList \ +// RUN: -analyzer-disable-checker=core.CallAndMessage \ +// RUN: -analyzer-output=text +// +// RUN: %clang_analyze_cc1 -triple x86_64-pc-linux-gnu -std=c23 -verify %s \ +// RUN: -analyzer-checker=core,security.VAList \ +// RUN: -analyzer-disable-checker=core.CallAndMessage \ +// RUN: -analyzer-output=text +// +// RUN: %clang_analyze_cc1 -triple x86_64-pc-linux-gnu -std=c23 %s \ +// RUN: -analyzer-checker=core,security.VAList + +// Test that the security.VAList checker recognizes __builtin_c23_va_start, +// which is used when compiling in C23 mode. + +#include "Inputs/system-header-simulator-for-valist-c23.h" + +void c23_no_warning(int fst, ...) { + va_list va; + va_start(va, fst); + (void)va_arg(va, int); + va_end(va); +} // no-warning + +void c23_one_arg_no_warning(int fst, ...) { + va_list va; + va_start(va); + (void)va_arg(va, int); + va_end(va); +} // no-warning + +void c23_uninitialized(int fst, ...) { + va_list va; + (void)va_arg(va, int); // expected-warning{{va_arg() is called on an uninitialized va_list}} + // expected-note@-1{{va_arg() is called on an uninitialized va_list}} +} + +void c23_use_after_end(int fst, ...) { + va_list va; + va_start(va, fst); // expected-note{{Initialized va_list}} + va_end(va); // expected-note{{Ended va_list}} + (void)va_arg(va, int); // expected-warning{{va_arg() is called on an already released va_list}} + // expected-note@-1{{va_arg() is called on an already released va_list}} +} + +void c23_copy(int fst, ...) { + va_list va, va2; + va_start(va, fst); + va_copy(va2, va); + va_end(va); + (void)va_arg(va2, int); + va_end(va2); +} // no-warning + +void c23_vprintf(int isstring, ...) { + va_list va; + va_start(va, isstring); + vprintf(isstring ? "%s" : "%d", va); + va_end(va); +} // no-warning diff --git a/clang/test/Analysis/valist-unterminated-c23.c b/clang/test/Analysis/valist-unterminated-c23.c new file mode 100644 index 0000000000000..8f1aae919239f --- /dev/null +++ b/clang/test/Analysis/valist-unterminated-c23.c @@ -0,0 +1,35 @@ +// RUN: %clang_analyze_cc1 -triple hexagon-unknown-linux -std=c23 -verify %s \ +// RUN: -analyzer-checker=core,security.VAList \ +// RUN: -analyzer-output=text +// +// RUN: %clang_analyze_cc1 -triple x86_64-pc-linux-gnu -std=c23 -verify %s \ +// RUN: -analyzer-checker=core,security.VAList \ +// RUN: -analyzer-output=text + +// Test that the security.VAList checker detects leaks with __builtin_c23_va_start. + +#include "Inputs/system-header-simulator-for-valist-c23.h" + +void c23_leak(int fst, ...) { + va_list va; + va_start(va, fst); // expected-note{{Initialized va_list}} + return; // expected-warning{{Initialized va_list 'va' is leaked}} + // expected-note@-1{{Initialized va_list 'va' is leaked}} +} + +void c23_reinit(int fst, ...) { + va_list va; + va_start(va, fst); // expected-note{{Initialized va_list}} + // expected-note@-1{{Initialized va_list}} + va_start(va, fst); // expected-warning{{Initialized va_list 'va' is initialized again}} + // expected-note@-1{{Initialized va_list 'va' is initialized again}} +} // expected-warning{{Initialized va_list 'va' is leaked}} + // expected-note@-1{{Initialized va_list 'va' is leaked}} + +void c23_reinit_ok(int fst, ...) { + va_list va; + va_start(va, fst); + va_end(va); + va_start(va, fst); + va_end(va); +} // no-warning `````````` </details> https://github.com/llvm/llvm-project/pull/192024 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
