NoQ created this revision.
NoQ added a reviewer: dcoughlin.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus,
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: clang.
The problem that was fixed in D25326 <https://reviews.llvm.org/D25326> for
inlined functions keeps biting us for the top-level functions as well. Namely,
i had to use `check::PreStmt<ReturnStmt>` rather than `check::EndFunction` in
D57558 <https://reviews.llvm.org/D57558> because in the `basic_test` test the
paths get merged before invoking the `check::EndFunction` callback. I don't
have an immediate fix for the whole overall problem, so for now, as a hack,
`check::PreStmt<ReturnStmt>` keeps being used.
However, the problem with `check::PreStmt<ReturnStmt>` is that automatic
destructors for function-local variables are not yet evaluated when this
callback is invoked. This causes false negatives in MIGChecker: if memory is
released in an automatic destructor and then an error code is returned, the bug
is not found because the Static Analyzer evalues the destructor *after* the
pre-return. Arguably, this may also be treated as a bug, depending on what does
the "Pre" in `PreStmt<ReturnStmt>` stand for.
But regardless, for now it seems that we'd have to subscribe on both callbacks.
Repository:
rC Clang
https://reviews.llvm.org/D58392
Files:
clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
clang/test/Analysis/mig.cpp
Index: clang/test/Analysis/mig.cpp
===================================================================
--- clang/test/Analysis/mig.cpp
+++ clang/test/Analysis/mig.cpp
@@ -44,3 +44,28 @@
return ret; // expected-warning{{MIG callback fails with error after
deallocating argument value. This is use-after-free vulnerability because
caller will try to deallocate it again}}
// expected-note@-1{{MIG callback fails with error after
deallocating argument value. This is use-after-free vulnerability because
caller will try to deallocate it again}}
}
+
+// Make sure we find the bug when the object is destroyed within an
+// automatic destructor.
+MIG_SERVER_ROUTINE
+kern_return_t test_vm_deallocate_in_automatic_dtor(mach_port_name_t port,
vm_address_t address, vm_size_t size) {
+ struct WillDeallocate {
+ mach_port_name_t port;
+ vm_address_t address;
+ vm_size_t size;
+ ~WillDeallocate() {
+ vm_deallocate(port, address, size); // expected-note{{Deallocating
object passed through parameter 'address'}}
+ }
+ } will_deallocate{port, address, size};
+
+ if (size > 10) {
+ // expected-note@-1{{Assuming 'size' is > 10}}
+ // expected-note@-2{{Taking true branch}}
+ return KERN_ERROR;
+ // expected-note@-1{{Calling '~WillDeallocate'}}
+ // expected-note@-2{{Returning from '~WillDeallocate'}}
+ // expected-warning@-3{{MIG callback fails with error after deallocating
argument value. This is use-after-free vulnerability because caller will try to
deallocate it again}}
+ // expected-note@-4 {{MIG callback fails with error after deallocating
argument value. This is use-after-free vulnerability because caller will try to
deallocate it again}}
+ }
+ return KERN_SUCCESS;
+}
Index: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -31,15 +31,30 @@
using namespace ento;
namespace {
-class MIGChecker : public Checker<check::PostCall, check::PreStmt<ReturnStmt>>
{
+class MIGChecker : public Checker<check::PostCall, check::PreStmt<ReturnStmt>,
+ check::EndFunction> {
BugType BT{this, "Use-after-free (MIG calling convention violation)",
categories::MemoryError};
CallDescription vm_deallocate { "vm_deallocate", 3 };
+ void checkReturnAux(const ReturnStmt *RS, CheckerContext &C) const;
+
public:
void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
- void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const;
+
+ // HACK: We're making two attempts to find the bug: checkEndFunction
+ // should normally be enough but it fails when the return value is a literal
+ // that never gets put into the Environment and ends of function with
multiple
+ // returns get agglutinated across returns, preventing us from obtaining
+ // the return value. The problem is similar to
https://reviews.llvm.org/D25326
+ // but now we step into it in the top-level function.
+ void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const {
+ checkReturnAux(RS, C);
+ }
+ void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const {
+ checkReturnAux(RS, C);
+ }
};
} // end anonymous namespace
@@ -103,7 +118,7 @@
C.addTransition(C.getState()->set<ReleasedParameter>(true), T);
}
-void MIGChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const {
+void MIGChecker::checkReturnAux(const ReturnStmt *RS, CheckerContext &C) const
{
// It is very unlikely that a MIG callback will be called from anywhere
// within the project under analysis and the caller isn't itself a routine
// that follows the MIG calling convention. Therefore we're safe to believe
Index: clang/test/Analysis/mig.cpp
===================================================================
--- clang/test/Analysis/mig.cpp
+++ clang/test/Analysis/mig.cpp
@@ -44,3 +44,28 @@
return ret; // expected-warning{{MIG callback fails with error after deallocating argument value. This is use-after-free vulnerability because caller will try to deallocate it again}}
// expected-note@-1{{MIG callback fails with error after deallocating argument value. This is use-after-free vulnerability because caller will try to deallocate it again}}
}
+
+// Make sure we find the bug when the object is destroyed within an
+// automatic destructor.
+MIG_SERVER_ROUTINE
+kern_return_t test_vm_deallocate_in_automatic_dtor(mach_port_name_t port, vm_address_t address, vm_size_t size) {
+ struct WillDeallocate {
+ mach_port_name_t port;
+ vm_address_t address;
+ vm_size_t size;
+ ~WillDeallocate() {
+ vm_deallocate(port, address, size); // expected-note{{Deallocating object passed through parameter 'address'}}
+ }
+ } will_deallocate{port, address, size};
+
+ if (size > 10) {
+ // expected-note@-1{{Assuming 'size' is > 10}}
+ // expected-note@-2{{Taking true branch}}
+ return KERN_ERROR;
+ // expected-note@-1{{Calling '~WillDeallocate'}}
+ // expected-note@-2{{Returning from '~WillDeallocate'}}
+ // expected-warning@-3{{MIG callback fails with error after deallocating argument value. This is use-after-free vulnerability because caller will try to deallocate it again}}
+ // expected-note@-4 {{MIG callback fails with error after deallocating argument value. This is use-after-free vulnerability because caller will try to deallocate it again}}
+ }
+ return KERN_SUCCESS;
+}
Index: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -31,15 +31,30 @@
using namespace ento;
namespace {
-class MIGChecker : public Checker<check::PostCall, check::PreStmt<ReturnStmt>> {
+class MIGChecker : public Checker<check::PostCall, check::PreStmt<ReturnStmt>,
+ check::EndFunction> {
BugType BT{this, "Use-after-free (MIG calling convention violation)",
categories::MemoryError};
CallDescription vm_deallocate { "vm_deallocate", 3 };
+ void checkReturnAux(const ReturnStmt *RS, CheckerContext &C) const;
+
public:
void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
- void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const;
+
+ // HACK: We're making two attempts to find the bug: checkEndFunction
+ // should normally be enough but it fails when the return value is a literal
+ // that never gets put into the Environment and ends of function with multiple
+ // returns get agglutinated across returns, preventing us from obtaining
+ // the return value. The problem is similar to https://reviews.llvm.org/D25326
+ // but now we step into it in the top-level function.
+ void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const {
+ checkReturnAux(RS, C);
+ }
+ void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const {
+ checkReturnAux(RS, C);
+ }
};
} // end anonymous namespace
@@ -103,7 +118,7 @@
C.addTransition(C.getState()->set<ReleasedParameter>(true), T);
}
-void MIGChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const {
+void MIGChecker::checkReturnAux(const ReturnStmt *RS, CheckerContext &C) const {
// It is very unlikely that a MIG callback will be called from anywhere
// within the project under analysis and the caller isn't itself a routine
// that follows the MIG calling convention. Therefore we're safe to believe
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
