[cfe-commits] r86538 - in /cfe/trunk: lib/Analysis/CMakeLists.txt lib/Analysis/GRExprEngineInternalChecks.cpp lib/Analysis/GRExprEngineInternalChecks.h lib/Analysis/PointerArithChecker.cpp test/Analysis/ptr-arith.c
Ted Kremenek
kremenek at apple.com
Mon Nov 9 17:48:19 PST 2009
Hi Zhongxing,
I'm not convinced this is a good check, as there are a bunch of valid
cases where you would do this.
First, I think this has the potential to flag a bunch of false
positives on OO code, like Gtk+. One thing I've noticed is that some
C code tries to compute the start of the object from the address of a
field (using offsetof). That seems reasonable to me, and appears
frequently in certain kinds of code bases.
Second, consider C++ iterators. For example, in our own code base we
might write:
SourceRange R = ...
foo(&R, &R+1);
where the '&R+1' represents the "end" iterator. This is a case where
a variable is treated as an array of size 1. We'd flag a warning
every time this idiom appears. That doesn't seem useful.
It seems to me that buffer overflow checking, where an actual load/
store is done on a invalid offset, will catch more real issues. Are
there cases that we will miss with buffer overflow checking that this
one will cover?
Ted
On Nov 9, 2009, at 5:23 AM, Zhongxing Xu wrote:
> Author: zhongxingxu
> Date: Mon Nov 9 07:23:31 2009
> New Revision: 86538
>
> URL: http://llvm.org/viewvc/llvm-project?rev=86538&view=rev
> Log:
> Add check for pointer arithmetic on non-array variables.
>
> Added:
> cfe/trunk/lib/Analysis/PointerArithChecker.cpp
> Modified:
> cfe/trunk/lib/Analysis/CMakeLists.txt
> cfe/trunk/lib/Analysis/GRExprEngineInternalChecks.cpp
> cfe/trunk/lib/Analysis/GRExprEngineInternalChecks.h
> cfe/trunk/test/Analysis/ptr-arith.c
>
> Modified: cfe/trunk/lib/Analysis/CMakeLists.txt
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CMakeLists.txt?rev=86538&r1=86537&r2=86538&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/lib/Analysis/CMakeLists.txt (original)
> +++ cfe/trunk/lib/Analysis/CMakeLists.txt Mon Nov 9 07:23:31 2009
> @@ -38,6 +38,7 @@
> NSAutoreleasePoolChecker.cpp
> NSErrorChecker.cpp
> PathDiagnostic.cpp
> + PointerArithChecker.cpp
> PointerSubChecker.cpp
> RangeConstraintManager.cpp
> RegionStore.cpp
>
> Modified: cfe/trunk/lib/Analysis/GRExprEngineInternalChecks.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/GRExprEngineInternalChecks.cpp?rev=86538&r1=86537&r2=86538&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/lib/Analysis/GRExprEngineInternalChecks.cpp (original)
> +++ cfe/trunk/lib/Analysis/GRExprEngineInternalChecks.cpp Mon Nov 9
> 07:23:31 2009
> @@ -413,6 +413,7 @@
> RegisterReturnStackAddressChecker(*this);
> RegisterReturnUndefChecker(*this);
> RegisterPointerSubChecker(*this);
> + RegisterPointerArithChecker(*this);
> RegisterFixedAddressChecker(*this);
> // Note that this must be registered after
> ReturnStackAddressChecker.
> RegisterReturnPointerRangeChecker(*this);
>
> Modified: cfe/trunk/lib/Analysis/GRExprEngineInternalChecks.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/GRExprEngineInternalChecks.h?rev=86538&r1=86537&r2=86538&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/lib/Analysis/GRExprEngineInternalChecks.h (original)
> +++ cfe/trunk/lib/Analysis/GRExprEngineInternalChecks.h Mon Nov 9
> 07:23:31 2009
> @@ -25,6 +25,7 @@
> void RegisterReturnUndefChecker(GRExprEngine &Eng);
> void RegisterVLASizeChecker(GRExprEngine &Eng);
> void RegisterPointerSubChecker(GRExprEngine &Eng);
> +void RegisterPointerArithChecker(GRExprEngine &Eng);
> void RegisterFixedAddressChecker(GRExprEngine &Eng);
> void RegisterCastToStructChecker(GRExprEngine &Eng);
> } // end clang namespace
>
> Added: cfe/trunk/lib/Analysis/PointerArithChecker.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/PointerArithChecker.cpp?rev=86538&view=auto
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/lib/Analysis/PointerArithChecker.cpp (added)
> +++ cfe/trunk/lib/Analysis/PointerArithChecker.cpp Mon Nov 9
> 07:23:31 2009
> @@ -0,0 +1,72 @@
> +//=== PointerArithChecker.cpp - Pointer arithmetic checker -----*- C
> ++ -*--===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open
> Source
> +// License. See LICENSE.TXT for details.
> +//
> +//
> =
> =
> =
> ----------------------------------------------------------------------=
> ==//
> +//
> +// This files defines PointerArithChecker, a builtin checker that
> checks for
> +// pointer arithmetic on locations other than array elements.
> +//
> +//
> =
> =
> =
> ----------------------------------------------------------------------=
> ==//
> +
> +#include "clang/Analysis/PathSensitive/CheckerVisitor.h"
> +#include "GRExprEngineInternalChecks.h"
> +
> +using namespace clang;
> +
> +namespace {
> +class VISIBILITY_HIDDEN PointerArithChecker
> + : public CheckerVisitor<PointerArithChecker> {
> + BuiltinBug *BT;
> +public:
> + PointerArithChecker() : BT(0) {}
> + static void *getTag();
> + void PreVisitBinaryOperator(CheckerContext &C, const
> BinaryOperator *B);
> +};
> +}
> +
> +void *PointerArithChecker::getTag() {
> + static int x;
> + return &x;
> +}
> +
> +void PointerArithChecker::PreVisitBinaryOperator(CheckerContext &C,
> + const
> BinaryOperator *B) {
> + if (B->getOpcode() != BinaryOperator::Sub &&
> + B->getOpcode() != BinaryOperator::Add)
> + return;
> +
> + const GRState *state = C.getState();
> + SVal LV = state->getSVal(B->getLHS());
> + SVal RV = state->getSVal(B->getRHS());
> +
> + const MemRegion *LR = LV.getAsRegion();
> +
> + if (!LR || !RV.isConstant())
> + return;
> +
> + // If pointer arithmetic is done on variables of non-array type,
> this often
> + // means behavior rely on memory organization, which is dangerous.
> + if (isa<VarRegion>(LR) || isa<CodeTextRegion>(LR) ||
> + isa<CompoundLiteralRegion>(LR)) {
> +
> + if (ExplodedNode *N = C.GenerateNode(B)) {
> + if (!BT)
> + BT = new BuiltinBug("Dangerous pointer arithmetic",
> + "Pointer arithmetic done on non-array
> variables "
> + "means reliance on memory layout, which
> is "
> + "dangerous.");
> + RangedBugReport *R = new RangedBugReport(*BT,BT-
> >getDescription().c_str(),
> + N);
> + R->addRange(B->getSourceRange());
> + C.EmitReport(R);
> + }
> + }
> +}
> +
> +void clang::RegisterPointerArithChecker(GRExprEngine &Eng) {
> + Eng.registerCheck(new PointerArithChecker());
> +}
>
> Modified: cfe/trunk/test/Analysis/ptr-arith.c
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/ptr-arith.c?rev=86538&r1=86537&r2=86538&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/test/Analysis/ptr-arith.c (original)
> +++ cfe/trunk/test/Analysis/ptr-arith.c Mon Nov 9 07:23:31 2009
> @@ -41,3 +41,12 @@
> int *p;
> p = (int*) 0x10000; // expected-warning{{Using a fixed address is
> not portable because that address will probably not be valid in all
> environments or platforms.}}
> }
> +
> +void f5() {
> + int x, y;
> + int *p;
> + p = &x + 1; // expected-warning{{Pointer arithmetic done on non
> array variables means reliance on memory layout, which is dangerous.}}
> +
> + int a[10];
> + p = a + 1; // no-warning
> +}
>
>
> _______________________________________________
> 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