[PATCH] Illegal delete/free not detected by clang static analyzer(MallocChecker)
Karthik Bhat
blitz.opensource at gmail.com
Tue Aug 6 22:56:11 PDT 2013
Thanks Jordan for the inputs. Please find the patch with review comments -
Index: lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp (revision 187716)
+++ lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp (working copy)
@@ -28,13 +28,16 @@
namespace {
class CallAndMessageChecker
- : public Checker< check::PreStmt<CallExpr>, check::PreObjCMessage,
+ : public Checker< check::PreStmt<CallExpr>,
+ check::PreStmt<CXXDeleteExpr>,
+ check::PreObjCMessage,
check::PreCall > {
mutable OwningPtr<BugType> BT_call_null;
mutable OwningPtr<BugType> BT_call_undef;
mutable OwningPtr<BugType> BT_cxx_call_null;
mutable OwningPtr<BugType> BT_cxx_call_undef;
mutable OwningPtr<BugType> BT_call_arg;
+ mutable OwningPtr<BugType> BT_cxx_delete_undef;
mutable OwningPtr<BugType> BT_msg_undef;
mutable OwningPtr<BugType> BT_objc_prop_undef;
mutable OwningPtr<BugType> BT_objc_subscript_undef;
@@ -44,6 +47,7 @@
public:
void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
+ void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const;
void checkPreObjCMessage(const ObjCMethodCall &msg, CheckerContext &C)
const;
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
@@ -250,6 +254,34 @@
C.addTransition(StNonNull);
}
+void CallAndMessageChecker::checkPreStmt(const CXXDeleteExpr *DE,
+ CheckerContext &C) const {
+
+ SVal Arg = C.getSVal(DE->getArgument());
+ FunctionDecl* FD = DE->getOperatorDelete();
+ if (!FD)
+ return;
+ if (Arg.isUndef()) {
+ StringRef Desc;
+ OverloadedOperatorKind Kind = FD->getOverloadedOperator();
+ ExplodedNode *N = C.generateSink();
+ if (!N)
+ return;
+ if (!BT_cxx_delete_undef)
+ BT_cxx_delete_undef.reset(new BuiltinBug("Uninitialized argument
value"));
+ if (Kind == OO_Array_Delete)
+ Desc = "Argument to delete[] is uninitialized";
+ else
+ Desc = "Argument to delete is uninitialized";
+ BugType *BT = BT_cxx_delete_undef.get();
+ BugReport *R = new BugReport(*BT, Desc, N);
+ bugreporter::trackNullOrUndefValue(N, DE, *R);
+ C.emitReport(R);
+ return;
+ }
+}
+
+
void CallAndMessageChecker::checkPreCall(const CallEvent &Call,
CheckerContext &C) const {
ProgramStateRef State = C.getState();
Index: test/Analysis/NewDelete-checker-test.cpp
===================================================================
--- test/Analysis/NewDelete-checker-test.cpp (revision 187717)
+++ test/Analysis/NewDelete-checker-test.cpp (working copy)
@@ -4,6 +4,7 @@
typedef __typeof__(sizeof(int)) size_t;
extern "C" void *malloc(size_t);
+extern "C" void free (void* ptr);
int *global;
//------------------
@@ -207,7 +208,38 @@
escapeVoidPtr(y);
} // no-warning
+//============== Test Uninitialized delete delete[]========================
+void testUninitDelete() {
+ int *x;
+ int * y = new int;
+ delete x; // expected-warning{{Argument to delete is uninitialized}}
+ delete y;
+}
+void testUninitDeleteArray() {
+ int *x;
+ int * y = new int[5];
+ delete[] x; // expected-warning{{Argument to delete[] is uninitialized}}
+ delete[] y;
+}
+
+void testUninitFree() {
+ int *x;
+ free(x); // expected-warning{{Function call argument is an uninitialized
value}}
+}
+
+void testUninitDeleteSink() {
+ int *x;
+ delete x; // expected-warning{{Argument to delete is uninitialized}}
+ (*(volatile int *)0 = 1); // no warn
+}
+
+void testUninitDeleteArraySink() {
+ int *x;
+ delete[] x; // expected-warning{{Argument to delete[] is uninitialized}}
+ (*(volatile int *)0 = 1); // no warn
+}
+
namespace reference_count {
class control_block {
unsigned count;
Regards
Karthik Bhat
On Tue, Aug 6, 2013 at 9:27 PM, Jordan Rose <jordan_rose at apple.com> wrote:
> Looking good! Some inline comments on the patch itself, now.
>
> On Aug 5, 2013, at 21:16 , Karthik Bhat <blitz.opensource at gmail.com>
> wrote:
>
> > Hi All,
> > Yes Jordan as you pointed out in the current infrastructure doesn't
> allow analyzer to conclude if a condition is true along all paths. I will
> have a look into core if it is possible to achieve the same. Currently i
> have backed out delete null case.
> >
> > Added patch for "Argument to delete is uninitialized" . Added test case
> for the same.
> >
> >
> > Index: lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
> > ===================================================================
> > --- lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp (revision
> 187716)
> > +++ lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp (working
> copy)
> > @@ -28,13 +28,16 @@
> >
> > namespace {
> > class CallAndMessageChecker
> > - : public Checker< check::PreStmt<CallExpr>, check::PreObjCMessage,
> > + : public Checker< check::PreStmt<CallExpr>,
> > + check::PreStmt<CXXDeleteExpr>,
> > + check::PreObjCMessage,
> > check::PreCall > {
> > mutable OwningPtr<BugType> BT_call_null;
> > mutable OwningPtr<BugType> BT_call_undef;
> > mutable OwningPtr<BugType> BT_cxx_call_null;
> > mutable OwningPtr<BugType> BT_cxx_call_undef;
> > mutable OwningPtr<BugType> BT_call_arg;
> > + mutable OwningPtr<BugType> BT_cxx_arg_undef;
>
> The name is missing the word "delete". Maybe BT_cxx_delete_undef?
>
>
> > mutable OwningPtr<BugType> BT_msg_undef;
> > mutable OwningPtr<BugType> BT_objc_prop_undef;
> > mutable OwningPtr<BugType> BT_objc_subscript_undef;
> > @@ -44,6 +47,7 @@
> > public:
> >
> > void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
> > + void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const;
>
> Indentation is off.
>
>
> > void checkPreObjCMessage(const ObjCMethodCall &msg, CheckerContext
> &C) const;
> > void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
> >
> > @@ -57,6 +61,9 @@
> > void emitNilReceiverBug(CheckerContext &C, const ObjCMethodCall &msg,
> > ExplodedNode *N) const;
> >
> > + void ReportBadDelete(const CXXDeleteExpr *DE, CheckerContext &C,
> > + BugType *BT, StringRef Desc, ExplodedNode *N)
> const;
>
> Lowercase names for functions. (Yes, I see the one right below that
> doesn't follow this convention.) Also, since this is no longer handling
> different types of bad delete besides 'delete' and 'delete[]', maybe it
> makes sense to just fold it into the check method, like checkPreCall.
>
>
> > void HandleNilReceiver(CheckerContext &C,
> > ProgramStateRef state,
> > const ObjCMethodCall &msg) const;
> > @@ -250,6 +257,32 @@
> > C.addTransition(StNonNull);
> > }
> >
> > +void CallAndMessageChecker::ReportBadDelete(const CXXDeleteExpr *DE,
> > + CheckerContext &C,
> > + BugType *BT,
> > + StringRef Desc,
> > + ExplodedNode *N) const {
> > +
> > + BugReport *R = new BugReport(*BT, Desc, N);
> > + bugreporter::trackNullOrUndefValue(N, DE, *R);
> > + C.emitReport(R);
> > +}
> > +
> > +void CallAndMessageChecker::checkPreStmt(const CXXDeleteExpr *DE,
> > + CheckerContext &C) const {
>
> Parameters not lined up?
>
> > +
> > + SVal RHS = C.getSVal(DE->getArgument());
>
> Why 'RHS'? A 'delete' expression doesn't exactly have a "left-hand side"
> and "right-hand side".
>
> > + if (DE && RHS.isUndef()) {
>
> You've already used DE unconditionally, so you can just drop that
> condition.
>
> > + ExplodedNode *N = C.generateSink();
>
> It's possible that we've seen this state before, in which case 'N' will be
> null. Please check for null and return early if necessary
>
> > + if (!BT_cxx_arg_undef)
> > + BT_cxx_arg_undef.reset(new BugType("Bad delete", "Memory Error"));
>
> This isn't really a memory error. I'd just go with BuiltinBug like the
> rest of the errors in this file.
>
> > + StringRef Desc = "Argument to delete is uninitialized";
>
> Let's take the opportunity here to say "Argument to 'delete'" or "Argument
> to 'delete[]'" depending on how the delete was written. That kind of polish
> goes a long way.
>
> > + ReportBadDelete(DE,C,BT_cxx_arg_undef.get(),Desc,N);
>
> Please follow the file's convention in putting spaces after commas.
> (Although again, it might make sense to just inline this function here.)
>
> > + return;
> > + }
> > +}
> > +
> > +
> > void CallAndMessageChecker::checkPreCall(const CallEvent &Call,
> > CheckerContext &C) const {
> > ProgramStateRef State = C.getState();
> > Index: test/Analysis/NewDelete-checker-test.cpp
> > ===================================================================
> > --- test/Analysis/NewDelete-checker-test.cpp (revision 187717)
> > +++ test/Analysis/NewDelete-checker-test.cpp (working copy)
> > @@ -4,6 +4,7 @@
> >
> > typedef __typeof__(sizeof(int)) size_t;
> > extern "C" void *malloc(size_t);
> > +extern "C" void free (void* ptr);
> > int *global;
> >
> > //------------------
> > @@ -207,7 +208,26 @@
> > escapeVoidPtr(y);
> > } // no-warning
> >
> > +//============== Test Uninitialized delete
> delete[]========================
> > +void testUninitDelete() {
> > + int *x;
> > + int * y = new int;
> > + delete x; // expected-warning{{Argument to delete is uninitialized}}
> > + delete y;
> > +}
>
> Since you've declared the uninitialized delete to be a fatal error, we're
> never even getting to the second delete. That's actually something work
> checking: do a bad delete, then immediately do a null pointer dereference
> or something (*(volatile int *)0 = 1). You should get no warning on the
> second line.
>
> That does mean that you're going to want to test the good case first.
>
>
> > +void testUninitDeleteArray() {
> > + int *x;
> > + int * y = new int[5];
> > + delete[] x; // expected-warning{{Argument to delete is uninitialized}}
> > + delete[] y;
> > +}
> > +
> > +void testUninitFree() {
> > + int *x;
> > + free(x); // expected-warning{{Function call argument is an
> uninitialized value}}
> > +}
> > +
>
> Thanks for working on this! It's almost ready to go in.
>
> Jordan
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130807/1b569b28/attachment.html>
More information about the cfe-commits
mailing list