From 4253051c16d0c2a5ae13af3d22383b61071ecb4c Mon Sep 17 00:00:00 2001 From: Ted Kremenek Date: Wed, 6 May 2009 18:19:24 +0000 Subject: [PATCH] Fix analyzer regression reported in PR 4164: - Update the old StoreManager::CastRegion to strip off 'ElementRegions' when casting to void* (Zhongxing: please validate) - Pass-by-reference argument invalidation logic in CFRefCount.cpp: - Strip ElementRegions when the ElementRegion is just a 'raw data' view on top of the underlying typed region. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@71094 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Analysis/CFRefCount.cpp | 24 ++++++++++++++++++++- lib/Analysis/Store.cpp | 43 ++++++++++++++++++++++++++----------- test/Analysis/pr_4164.c | 41 +++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 13 deletions(-) create mode 100644 test/Analysis/pr_4164.c diff --git a/lib/Analysis/CFRefCount.cpp b/lib/Analysis/CFRefCount.cpp index ca420006d2..4c517fd537 100644 --- a/lib/Analysis/CFRefCount.cpp +++ b/lib/Analysis/CFRefCount.cpp @@ -2606,7 +2606,29 @@ void CFRefCount::EvalSummary(ExplodedNodeSet& Dst, const TypedRegion* R = dyn_cast(MR->getRegion()); - if (R) { + if (R) { + // Are we dealing with an ElementRegion? If the element type is + // a basic integer type (e.g., char, int) and the underying region + // is also typed then strip off the ElementRegion. + // FIXME: We really need to think about this for the general case + // as sometimes we are reasoning about arrays and other times + // about (char*), etc., is just a form of passing raw bytes. + // e.g., void *p = alloca(); foo((char*)p); + if (const ElementRegion *ER = dyn_cast(R)) { + // Checking for 'integral type' is probably too promiscuous, but + // we'll leave it in for now until we have a systematic way of + // handling all of these cases. Eventually we need to come up + // with an interface to StoreManager so that this logic can be + // approriately delegated to the respective StoreManagers while + // still allowing us to do checker-specific logic (e.g., + // invalidating reference counts), probably via callbacks. + if (ER->getElementType()->isIntegralType()) + if (const TypedRegion *superReg = + dyn_cast(ER->getSuperRegion())) + R = superReg; + // FIXME: What about layers of ElementRegions? + } + // Is the invalidated variable something that we were tracking? SymbolRef Sym = state.GetSValAsScalarOrLoc(R).getAsLocSymbol(); diff --git a/lib/Analysis/Store.cpp b/lib/Analysis/Store.cpp index 32b186b269..49411b468e 100644 --- a/lib/Analysis/Store.cpp +++ b/lib/Analysis/Store.cpp @@ -44,18 +44,37 @@ StoreManager::CastRegion(const GRState* state, const MemRegion* R, QualType Pointee = PTy->getPointeeType(); if (Pointee->isVoidType()) { - // Casts to void* only removes TypedViewRegion. If there is no - // TypedViewRegion, leave the region untouched. This happens when: - // - // void foo(void*); - // ... - // void bar() { - // int x; - // foo(&x); - // } - - if (const TypedViewRegion *TR = dyn_cast(R)) - R = TR->removeViews(); + do { + if (const TypedViewRegion *TR = dyn_cast(R)) { + // Casts to void* removes TypedViewRegion. This happens when: + // + // void foo(void*); + // ... + // void bar() { + // int x; + // foo(&x); + // } + // + R = TR->removeViews(); + continue; + } + else if (const ElementRegion *ER = dyn_cast(R)) { + // Casts to void* also removes ElementRegions. This happens when: + // + // void foo(void*); + // ... + // void bar() { + // int x; + // foo((char*)&x); + // } + // + R = ER->getSuperRegion(); + continue; + } + else + break; + } + while (0); return CastResult(state, R); } diff --git a/test/Analysis/pr_4164.c b/test/Analysis/pr_4164.c new file mode 100644 index 0000000000..cc2479c3e4 --- /dev/null +++ b/test/Analysis/pr_4164.c @@ -0,0 +1,41 @@ +// RUN: clang-cc -triple x86_64-apple-darwin9 -analyze -checker-cfref -analyzer-store=basic -verify %s && +// RUN: clang-cc -triple x86_64-apple-darwin9 -analyze -checker-cfref -analyzer-store=region -verify %s + +// PR 4164: http://llvm.org/bugs/show_bug.cgi?id=4164 +// +// Eventually this should be pulled into misc-ps.m. This is in a separate test +// file for now to play around with the specific issues for BasicStoreManager +// and StoreManager (i.e., we can make a copy of this file for either +// StoreManager should one start to fail in the near future). +// +// The basic issue is that the VarRegion for 'size' is casted to (char*), +// resulting in an ElementRegion. 'getsockopt' is an unknown function that +// takes a void*, which means the ElementRegion should get stripped off. +typedef unsigned int __uint32_t; +typedef __uint32_t __darwin_socklen_t; +typedef __darwin_socklen_t socklen_t; +int getsockopt(int, int, int, void * restrict, socklen_t * restrict); + +int test1() { + int s = -1; + int size; + socklen_t size_len = sizeof(size); + if (getsockopt(s, 0xffff, 0x1001, (char *)&size, &size_len) < 0) + return -1; + + return size; // no-warning +} + +// Similar case: instead of passing a 'void*', we pass 'char*'. In this +// case we pass an ElementRegion to the invalidation logic. Since it is +// an ElementRegion that just layers on top of another typed region and the +// ElementRegion itself has elements whose type are integral (essentially raw +// data) we strip off the ElementRegion when doing the invalidation. +int takes_charptr(char* p); +int test2() { + int size; + if (takes_charptr((char*)&size)) + return -1; + return size; // no-warning +} + -- 2.40.0