[PATCH] Illegal delete/free not detected by clang static analyzer(MallocChecker)
Jordan Rose
jordan_rose at apple.com
Mon Aug 5 09:09:19 PDT 2013
delete nullptr / delete 0 is entirely legal in C++. Can you please take that back out?
Jordan
On Aug 5, 2013, at 2:47 , Karthik Bhat <blitz.opensource at gmail.com> wrote:
> Hi Jordan,
> Thanks for the inputs. I have modified the same in CallAndMessageChecker to
> handle delete and delete[] cases.
> Added cases for the same in NewDelete-checker-test.cpp.
>
> Also there is one more issue not detected by new/delete checker.
> For a TC like -
> char* x = 0;
> delete x;
> we do not get Null dereference warning. Added patch to handle the same.
> Please let me know your inputs on 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,17 @@
>
> 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;
> + mutable OwningPtr<BugType> BT_cxx_arg_null;
> mutable OwningPtr<BugType> BT_msg_undef;
> mutable OwningPtr<BugType> BT_objc_prop_undef;
> mutable OwningPtr<BugType> BT_objc_subscript_undef;
> @@ -44,6 +48,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;
>
> @@ -57,6 +62,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;
> +
> void HandleNilReceiver(CheckerContext &C,
> ProgramStateRef state,
> const ObjCMethodCall &msg) const;
> @@ -250,6 +258,52 @@
> 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 {
> +
> + SVal RHS = C.getSVal(DE->getArgument());
> + if (DE && RHS.isUndef()) {
> + ExplodedNode *N = C.generateSink();
> + if (!BT_cxx_arg_undef)
> + BT_cxx_arg_undef.reset(new BugType("Bad delete", "Memory Error"));
> + StringRef Desc = "Argument to delete is uninitialized";
> + ReportBadDelete(DE,C,BT_cxx_arg_undef.get(),Desc,N);
> + return;
> + }
> +
> + DefinedOrUnknownSVal location = RHS.castAs<DefinedOrUnknownSVal>();
> + // Check for null dereferences.
> + if (!location.getAs<Loc>())
> + return;
> +
> + ProgramStateRef state = C.getState();
> +
> + ProgramStateRef notNullState, nullState;
> + llvm::tie(notNullState, nullState) = state->assume(location);
> + if (nullState) {
> + if (!notNullState) {
> + ExplodedNode *N = C.generateSink();
> + if (!BT_cxx_arg_null)
> + BT_cxx_arg_null.reset(new BugType("Null Dereference", "Memory
> Error"));
> + StringRef Desc = "Dereference of null pointer";
> + ReportBadDelete(DE,C,BT_cxx_arg_null.get(),Desc,N);
> + return;
> + }
> + }
> +}
> +
> +
> void CallAndMessageChecker::checkPreCall(const CallEvent &Call,
> CheckerContext &C) const {
> ProgramStateRef State = C.getState();
>
>
>
> Index: test/Analysis/inlining/containers.cpp
> ===================================================================
> --- test/Analysis/inlining/containers.cpp (revision 187717)
> +++ test/Analysis/inlining/containers.cpp (working copy)
> @@ -100,7 +100,7 @@
> #endif
> }
>
> - ~MySet() { delete[] storage; }
> + ~MySet() { if (storage) delete[] storage; }
>
> bool isEmpty() {
> clang_analyzer_checkInlined(true); // expected-warning {{TRUE}}
>
>
>
> 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,50 @@
> 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}}
> +}
> +
> +
> +//=============== Test for Null Dereference using
> delete=======================
> +void testNullDeref() {
> + int *x = 0;
> + delete x; // expected-warning{{Dereference of null pointer}}
> +}
> +
> +void testArrayNullDeref() {
> + int *x = 0;
> + delete[] x; // expected-warning{{Dereference of null pointer}}
> +}
> +
> +class derefTest {
> + int *storage;
> +public:
> + derefTest() : storage(0) {};
> + ~derefTest(){delete[] storage;} // expected-warning{{Dereference of null
> pointer}}
> +};
> +
> +// Destructor call will result in a Null Dereference
> +void testClassMemNullDeref() {
> + derefTest test;
> +}
> +
> namespace reference_count {
> class control_block {
> unsigned count;
>
>
>
>
> Regards
> Karthik
>
>
>
> On Fri, Aug 2, 2013 at 9:46 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>
>> The real warning here isn't "Freeing non-allocated memory", it's "Argument
>> to 'delete' is uninitialized". We already get this check for 'free' because
>> it looks like a function call, but right now the same is not true for
>> 'delete'.
>>
>> Eventually 'delete' will be modeled in terms of a destructor call and a
>> deallocator, and at that point we'd probably get this warning for free. For
>> now, though, this does seem useful.
>>
>> Please change the patch to only check 'delete' and 'delete[]', change the
>> message to talk about uninitialized memory (and possibly use SVal::isUndef
>> instead), and add tests for both 'delete' forms (plus a test for 'free' to
>> make sure the call checker is still doing its work).
>>
>> Thanks for tracking this down!
>> Jordan
>>
>>
>> On Aug 2, 2013, at 3:06 , Karthik Bhat <blitz.opensource at gmail.com> wrote:
>>
>>> Hi,
>>> In case we have a code like -
>>>
>>> int main() {
>>> char* p;
>>> delete p; // Illegal Delete
>>> }
>>>
>>> Illegal delete for statement "delete p" is not reported. Added a patch
>> to fix the same. Please let me know if the patch is ok.
>>>
>>> Index: test/Analysis/malloc-interprocedural.c
>>> ===================================================================
>>> --- test/Analysis/malloc-interprocedural.c (revision 187647)
>>> +++ test/Analysis/malloc-interprocedural.c (working copy)
>>> @@ -68,10 +68,13 @@
>>> my_free1((int*)data); // expected-warning{{Use of memory after it is
>> freed}}
>>> }
>>>
>>> +static void my_free2(void *p) {
>>> + free(p); // expected-warning{{Freeing a non allocated memory}}
>>> +}
>>> // TODO: We should warn here.
>>> void test5() {
>>> int *data;
>>> - my_free1((int*)data);
>>> + my_free2((int*)data);
>>> }
>>>
>>> static char *reshape(char *in) {
>>> Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
>>> ===================================================================
>>> --- lib/StaticAnalyzer/Checkers/MallocChecker.cpp (revision 187648)
>>> +++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp (working copy)
>>> @@ -961,8 +961,19 @@
>>> bool ReturnsNullOnFailure)
>> const {
>>>
>>> SVal ArgVal = State->getSVal(ArgExpr, C.getLocationContext());
>>> - if (!ArgVal.getAs<DefinedOrUnknownSVal>())
>>> + if (!ArgVal.getAs<DefinedOrUnknownSVal>()) {
>>> + if (ExplodedNode *N = C.addTransition(C.getState())) {
>>> + if (!BT_BadFree)
>>> + BT_BadFree.reset(new BugType("Bad free", "Memory Error"));
>>> + SmallString<100> buf;
>>> + llvm::raw_svector_ostream os(buf);
>>> + os << "Freeing a non allocated memory";
>>> + BugReport *R = new BugReport(*BT_BadFree, os.str(), N);
>>> + R->addRange(ArgExpr->getSourceRange());
>>> + C.emitReport(R);
>>> + }
>>> return 0;
>>> + }
>>> DefinedOrUnknownSVal location = ArgVal.castAs<DefinedOrUnknownSVal>();
>>>
>>>
>>> Thanks and Regards
>>> Karthik Bhat
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
More information about the cfe-commits
mailing list