Skip to content

[analyzer] Fix false double free when including 3rd-party headers with overloaded delete operator as system headers #85224

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 31, 2024
Merged

[analyzer] Fix false double free when including 3rd-party headers with overloaded delete operator as system headers #85224

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b3e6a22
wip: the first workaround
Snape3058 Mar 14, 2024
89ca329
Merge remote-tracking branch 'llvm/main' into issue-62985
steakhal May 5, 2024
d7e9b88
Cleanup test and impl
steakhal May 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Contributor

@steakhal steakhal Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check still has some problems.
FD is not guaranteed to be the function decl which has the body, thus consequently, the location of FD might not point to the definition, even if FD has a definition.

FD likely points to the last decl declaration spelling.
I didn't have time to publish my extra tests demonstrating this, but I'll come back to this.

Copy link
Member Author

@Snape3058 Snape3058 Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FD is not guaranteed to be the function decl which has the body

But FunctionDecl::hasBody will traverse all re-declarations and verify whether there is a decl has a body defined. So I think it will not affect the correctness no matter which re-declaration it points to.

My concern is the situation when the current TU does not have its definition but can be imported from other TUs during CTU inlining. Under this circumstance, the declaration can still pass the check here but trigger a false positive after CTU inlining.

+------------+   +------------------+   +----------------+
|checkPreCall|-->|No body in this TU|-->|Mark as Released|
+------------+   +------------------+   +----------------+
      |
      v
  +--------+   +---+   +----------------------+   +--------------------+
  |evalCall|-->|CTU|-->|Found body in other TU|-->|Inline external body|
  +--------+   +---+   +----------------------+   +--------------------+
      |
      v
+--------------------------------+   +-------------------------+
|the real delete in imported body|-->|double free (false alarm)|
+--------------------------------+   +-------------------------+

I am not sure whether there are other scenarios like this that can also trigger this false positive. If so, how can we update the conditions for these scenarios?

The current version is just a workaround for this bug we encountered here, rather than a robust verifier for checking whether a given overload is a standard one.

Copy link
Collaborator

@balazske balazske Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to move the check into checkPostCall? Then this situation can not happen.

Copy link
Contributor

@steakhal steakhal Mar 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an excellent question! :D

}

//===----------------------------------------------------------------------===//
Expand Down
17 changes: 17 additions & 0 deletions clang/test/Analysis/Inputs/overloaded-delete-in-header.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
25 changes: 25 additions & 0 deletions clang/test/Analysis/overloaded-delete-in-system-header.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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