https://github.com/steakhal updated https://github.com/llvm/llvm-project/pull/85224
>From b3e6a2273e9c35ac4cc3ac16aebcf4ea0d89ef74 Mon Sep 17 00:00:00 2001 From: Ella Ma <alansnape3...@gmail.com> Date: Thu, 14 Mar 2024 20:41:20 +0800 Subject: [PATCH 1/2] wip: the first workaround --- .../StaticAnalyzer/Checkers/MallocChecker.cpp | 3 ++- .../Inputs/overloaded-delete-in-header.h | 17 +++++++++++++ .../overloaded-delete-in-system-header.cpp | 25 +++++++++++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 clang/test/Analysis/Inputs/overloaded-delete-in-header.h create mode 100644 clang/test/Analysis/overloaded-delete-in-system-header.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 03cb7696707fe2..c7ec2b7cc43b30 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -1090,7 +1090,8 @@ static bool isStandardNewDelete(const FunctionDecl *FD) { // If the header for operator delete is not included, it's still defined // in an invalid source location. Check to make sure we don't crash. return !L.isValid() || - FD->getASTContext().getSourceManager().isInSystemHeader(L); + (!FD->hasBody() && // FIXME: Still a false alarm after CTU inlining. + FD->getASTContext().getSourceManager().isInSystemHeader(L)); } //===----------------------------------------------------------------------===// diff --git a/clang/test/Analysis/Inputs/overloaded-delete-in-header.h b/clang/test/Analysis/Inputs/overloaded-delete-in-header.h new file mode 100644 index 00000000000000..5090de0d9bbd6b --- /dev/null +++ b/clang/test/Analysis/Inputs/overloaded-delete-in-header.h @@ -0,0 +1,17 @@ +#ifndef OVERLOADED_DELETE_IN_HEADER +#define OVERLOADED_DELETE_IN_HEADER + +void clang_analyzer_printState(); + +struct DeleteInHeader { + inline void operator delete(void *ptr) { + // No matter whether this header file is included as a system header file + // with -isystem or a user header file with -I, ptr should not be marked as + // released. + clang_analyzer_printState(); + + ::operator delete(ptr); // The first place where ptr is marked as released. + } +}; + +#endif // OVERLOADED_DELETE_IN_SYSTEM_HEADER diff --git a/clang/test/Analysis/overloaded-delete-in-system-header.cpp b/clang/test/Analysis/overloaded-delete-in-system-header.cpp new file mode 100644 index 00000000000000..f7780b67e93b99 --- /dev/null +++ b/clang/test/Analysis/overloaded-delete-in-system-header.cpp @@ -0,0 +1,25 @@ +// issue 62985 +// When 3rd-party header files are included as system headers, their overloaded +// new and delete operators are also considered as the std ones. However, those +// overloaded operator functions will also be inlined. This makes the same +// symbolic memory marked as released twice, which leads to a false uaf alarm. +// +// The first run, include as system header. False uaf report before fix. +// +// RUN: %clang_analyze_cc1 %s \ +// RUN: -analyzer-checker=core,cplusplus.NewDelete,debug.ExprInspection \ +// RUN: -isystem %S/Inputs/ 2>&1 | \ +// RUN: FileCheck %s +// +// The second run, include as user header. Should always silent. +// +// RUN: %clang_analyze_cc1 %s \ +// RUN: -analyzer-checker=core,cplusplus.NewDelete,debug.ExprInspection \ +// RUN: -I %S/Inputs/ 2>&1 | \ +// RUN: FileCheck %s + +#include "overloaded-delete-in-header.h" + +void deleteInHeader(DeleteInHeader *p) { delete p; } + +// CHECK-NOT: Released >From d7e9b8867a45d329ef0f68fbafe8715acf8ad02f Mon Sep 17 00:00:00 2001 From: Balazs Benics <benicsbal...@gmail.com> Date: Sun, 5 May 2024 12:34:28 +0200 Subject: [PATCH 2/2] Cleanup test and impl I'd prefer using the definition of FD to check the beginning brace location. --- .../StaticAnalyzer/Checkers/MallocChecker.cpp | 8 +++--- .../Inputs/overloaded-delete-in-header.h | 21 ++++++++------- .../overloaded-delete-in-system-header.cpp | 26 ++++--------------- 3 files changed, 21 insertions(+), 34 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 1efc58600f9e63..6c18f2a1d9ee4a 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -1096,13 +1096,15 @@ static bool isStandardNewDelete(const FunctionDecl *FD) { Kind != OO_Array_Delete) return false; + bool HasBody = FD->hasBody(); // Prefer using the definition. + // This is standard if and only if it's not defined in a user file. SourceLocation L = FD->getLocation(); + // If the header for operator delete is not included, it's still defined // in an invalid source location. Check to make sure we don't crash. - return !L.isValid() || - (!FD->hasBody() && // FIXME: Still a false alarm after CTU inlining. - FD->getASTContext().getSourceManager().isInSystemHeader(L)); + const auto &SM = FD->getASTContext().getSourceManager(); + return L.isInvalid() || (!HasBody && SM.isInSystemHeader(L)); } //===----------------------------------------------------------------------===// diff --git a/clang/test/Analysis/Inputs/overloaded-delete-in-header.h b/clang/test/Analysis/Inputs/overloaded-delete-in-header.h index 5090de0d9bbd6b..8243961d84830b 100644 --- a/clang/test/Analysis/Inputs/overloaded-delete-in-header.h +++ b/clang/test/Analysis/Inputs/overloaded-delete-in-header.h @@ -1,17 +1,18 @@ #ifndef OVERLOADED_DELETE_IN_HEADER #define OVERLOADED_DELETE_IN_HEADER -void clang_analyzer_printState(); - struct DeleteInHeader { - inline void operator delete(void *ptr) { - // No matter whether this header file is included as a system header file - // with -isystem or a user header file with -I, ptr should not be marked as - // released. - clang_analyzer_printState(); - - ::operator delete(ptr); // The first place where ptr is marked as released. - } + int data; + static void operator delete(void *ptr); }; +void DeleteInHeader::operator delete(void *ptr) { + DeleteInHeader *self = (DeleteInHeader *)ptr; + self->data = 1; // no-warning: Still alive. + + ::operator delete(ptr); + + self->data = 2; // expected-warning {{Use of memory after it is freed [cplusplus.NewDelete]}} +} + #endif // OVERLOADED_DELETE_IN_SYSTEM_HEADER diff --git a/clang/test/Analysis/overloaded-delete-in-system-header.cpp b/clang/test/Analysis/overloaded-delete-in-system-header.cpp index f7780b67e93b99..c284a942063061 100644 --- a/clang/test/Analysis/overloaded-delete-in-system-header.cpp +++ b/clang/test/Analysis/overloaded-delete-in-system-header.cpp @@ -1,25 +1,9 @@ -// issue 62985 -// When 3rd-party header files are included as system headers, their overloaded -// new and delete operators are also considered as the std ones. However, those -// overloaded operator functions will also be inlined. This makes the same -// symbolic memory marked as released twice, which leads to a false uaf alarm. -// -// The first run, include as system header. False uaf report before fix. -// -// RUN: %clang_analyze_cc1 %s \ -// RUN: -analyzer-checker=core,cplusplus.NewDelete,debug.ExprInspection \ -// RUN: -isystem %S/Inputs/ 2>&1 | \ -// RUN: FileCheck %s -// -// The second run, include as user header. Should always silent. -// -// RUN: %clang_analyze_cc1 %s \ -// RUN: -analyzer-checker=core,cplusplus.NewDelete,debug.ExprInspection \ -// RUN: -I %S/Inputs/ 2>&1 | \ -// RUN: FileCheck %s +// RUN: %clang_analyze_cc1 -isystem %S/Inputs/ -verify %s \ +// RUN: -analyzer-checker=core,unix.Malloc,cplusplus.NewDelete + +// RUN: %clang_analyze_cc1 -I %S/Inputs/ -verify %s \ +// RUN: -analyzer-checker=core,unix.Malloc,cplusplus.NewDelete #include "overloaded-delete-in-header.h" void deleteInHeader(DeleteInHeader *p) { delete p; } - -// CHECK-NOT: Released _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits