[cfe-commits] r125640 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/Sema/SemaChecking.cpp lib/Sema/SemaExpr.cpp test/Analysis/out-of-bounds.c test/Sema/array-bounds.c

[Hans Wennborg]

On Wed, Feb 16, 2011 at 1:57 AM, Ted Kremenek <kremenek at apple.com> wrote:
> Author: kremenek
> Date: Tue Feb 15 19:57:07 2011
> New Revision: 125640
>
> URL: http://llvm.org/viewvc/llvm-project?rev=125640&view=rev
> Log:
> Add trivial buffer overflow checking in Sema.
>
> Added:
>    cfe/trunk/test/Sema/array-bounds.c
> Modified:
>    cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>    cfe/trunk/include/clang/Sema/Sema.h
>    cfe/trunk/lib/Sema/SemaChecking.cpp
>    cfe/trunk/lib/Sema/SemaExpr.cpp
>    cfe/trunk/test/Analysis/out-of-bounds.c
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=125640&r1=125639&r2=125640&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Feb 15 19:57:07 2011
> @@ -3379,6 +3379,10 @@
>  def warn_explicit_conversion_functions : Warning<
>   "explicit conversion functions are a C++0x extension">, InGroup<CXX0x>;
>
> +def warn_array_index_out_of_bounds : Warning<
> +  "array index %select{precedes first|excedes last}0 array element">,
> +  InGroup<DiagGroup<"array-bounds">>;
> +
>  def warn_printf_write_back : Warning<
>   "use of '%%n' in format string discouraged (potentially insecure)">,
>   InGroup<FormatSecurity>;
>
> Modified: cfe/trunk/include/clang/Sema/Sema.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=125640&r1=125639&r2=125640&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Sema/Sema.h (original)
> +++ cfe/trunk/include/clang/Sema/Sema.h Tue Feb 15 19:57:07 2011
> @@ -5056,7 +5056,8 @@
>   SourceLocation getLocationOfStringLiteralByte(const StringLiteral *SL,
>                                                 unsigned ByteNo) const;
>
> -private:
> +private:
> +  void CheckArrayAccess(const ArraySubscriptExpr *ae);
>   bool CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall);
>   bool CheckBlockCall(NamedDecl *NDecl, CallExpr *TheCall);
>
>
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=125640&r1=125639&r2=125640&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Tue Feb 15 19:57:07 2011
> @@ -3080,3 +3080,33 @@
>     << TRange << Op->getSourceRange();
>  }
>
> +void Sema::CheckArrayAccess(const clang::ArraySubscriptExpr *ae) {
> +  const DeclRefExpr *dr =
> +    dyn_cast<DeclRefExpr>(ae->getBase()->IgnoreParenImpCasts());
> +  if (!dr)
> +    return;
> +  const VarDecl *vd = cast<VarDecl>(dr->getDecl());
> +  const ConstantArrayType *cat = Context.getAsConstantArrayType(vd->getType());
> +  if (!cat)
> +    return;
> +  const Expr *idx = ae->getIdx();
> +  if (idx->isValueDependent())
> +    return;
> +  llvm::APSInt result;
> +  if (!idx->isIntegerConstantExpr(result, Context))
> +    return;
> +  unsigned kind = 2;
> +  if (result.slt(0))
> +    kind = /* precedes */ 0;
> +  else {
> +    const llvm::APInt &size = cat->getSize();
> +    if (size.getBitWidth() > result.getBitWidth())
> +      result = result.sext(size.getBitWidth());
> +    if (result.sge(size))
> +      kind = /* excedes */ 1;
> +  }
> +  if (kind < 2)
> +    Diag(ae->getBase()->getLocEnd(), diag::warn_array_index_out_of_bounds)
> +      << kind << idx->getSourceRange();
> +}
> +
>
> Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=125640&r1=125639&r2=125640&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaExpr.cpp Tue Feb 15 19:57:07 2011
> @@ -294,6 +294,9 @@
>   if (T.hasQualifiers())
>     T = T.getUnqualifiedType();
>
> +  if (const ArraySubscriptExpr *ae = dyn_cast<ArraySubscriptExpr>(E))
> +    CheckArrayAccess(ae);
> +
>   E = ImplicitCastExpr::Create(Context, T, CK_LValueToRValue,
>                                E, 0, VK_RValue);
>  }
> @@ -7242,6 +7245,11 @@
>     Diag(UO->getOperatorLoc(), diag::note_indirection_through_null);
>   }
>
> +  // Check for trivial buffer overflows.
> +  if (const ArraySubscriptExpr *ae
> +      = dyn_cast<ArraySubscriptExpr>(LHS->IgnoreParenCasts()))
> +    CheckArrayAccess(ae);
> +
>   // C99 6.5.16p3: The type of an assignment expression is the type of the
>   // left operand unless the left operand has qualified type, in which case
>   // it is the unqualified version of the type of the left operand.
>
> Modified: cfe/trunk/test/Analysis/out-of-bounds.c
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/out-of-bounds.c?rev=125640&r1=125639&r2=125640&view=diff
> ==============================================================================
> --- cfe/trunk/test/Analysis/out-of-bounds.c (original)
> +++ cfe/trunk/test/Analysis/out-of-bounds.c Tue Feb 15 19:57:07 2011
> @@ -1,4 +1,4 @@
> -// RUN: %clang_cc1 -analyze -analyzer-check-objc-mem -analyzer-check-buffer-overflows -verify %s
> +// RUN: %clang_cc1 -Wno-array-bounds -analyze -analyzer-check-objc-mem -analyzer-check-buffer-overflows -verify %s
>
>  // Tests doing an out-of-bounds access after the end of an array using:
>  // - constant integer index
>
> Added: cfe/trunk/test/Sema/array-bounds.c
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/array-bounds.c?rev=125640&view=auto
> ==============================================================================
> --- cfe/trunk/test/Sema/array-bounds.c (added)
> +++ cfe/trunk/test/Sema/array-bounds.c Tue Feb 15 19:57:07 2011
> @@ -0,0 +1,16 @@
> +// RUN: %clang_cc1 -verify %s
> +
> +int foo() {
> +  int x[2];
> +  int y[2];
> +  int *p = &y[2]; // no-warning
> +  (void) sizeof(x[2]); // no-warning
> +  y[2] = 2; // expected-warning{{array index excedes last array element}}
> +  return x[2] +  // expected-warning{{array index excedes last array element}}
> +         y[-1] + // expected-warning{{array index precedes first array element}}
> +         x[sizeof(x)] +  // expected-warning{{array index excedes last array element}}
> +         x[sizeof(x) / sizeof(x[0])] +  // expected-warning{{array index excedes last array element}}
> +         x[sizeof(x) / sizeof(x[0]) - 1] + // no-warning
> +         x[sizeof(x[2])]; // expected-warning{{array index excedes last array element}}
> +}
> +
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>

This breaks the Chrome build, where we have some code like this:

template <bool extendArray>
void myFunc() {
    int arr[3 + (extendArray ? 1 : 0)];

    if (extendArray)
        arr[3] = 42;
}

void f() {
    myFunc<false>();
}

(The real code is here:
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp#L245)

In fact, it seems to warn also in a case like this:

void f() {
    int arr[42];
    if (0)
        arr[100] = 5;
}

Would it be possible to make the warning a bit more conservative?


Hans