<br><br><div class="gmail_quote">On Thu, Feb 17, 2011 at 2:56 AM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org">hans@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div><div></div><div class="h5">On Wed, Feb 16, 2011 at 1:57 AM, Ted Kremenek <<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>> wrote:<br>
> Author: kremenek<br>
> Date: Tue Feb 15 19:57:07 2011<br>
> New Revision: 125640<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=125640&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=125640&view=rev</a><br>
> Log:<br>
> Add trivial buffer overflow checking in Sema.<br>
><br>
> Added:<br>
>    cfe/trunk/test/Sema/array-bounds.c<br>
> Modified:<br>
>    cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
>    cfe/trunk/include/clang/Sema/Sema.h<br>
>    cfe/trunk/lib/Sema/SemaChecking.cpp<br>
>    cfe/trunk/lib/Sema/SemaExpr.cpp<br>
>    cfe/trunk/test/Analysis/out-of-bounds.c<br>
><br>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=125640&r1=125639&r2=125640&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=125640&r1=125639&r2=125640&view=diff</a><br>

> ==============================================================================<br>
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Feb 15 19:57:07 2011<br>
> @@ -3379,6 +3379,10 @@<br>
>  def warn_explicit_conversion_functions : Warning<<br>
>   "explicit conversion functions are a C++0x extension">, InGroup<CXX0x>;<br>
><br>
> +def warn_array_index_out_of_bounds : Warning<<br>
> +  "array index %select{precedes first|excedes last}0 array element">,<br>
> +  InGroup<DiagGroup<"array-bounds">>;<br>
> +<br>
>  def warn_printf_write_back : Warning<<br>
>   "use of '%%n' in format string discouraged (potentially insecure)">,<br>
>   InGroup<FormatSecurity>;<br>
><br>
> Modified: cfe/trunk/include/clang/Sema/Sema.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=125640&r1=125639&r2=125640&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=125640&r1=125639&r2=125640&view=diff</a><br>

> ==============================================================================<br>
> --- cfe/trunk/include/clang/Sema/Sema.h (original)<br>
> +++ cfe/trunk/include/clang/Sema/Sema.h Tue Feb 15 19:57:07 2011<br>
> @@ -5056,7 +5056,8 @@<br>
>   SourceLocation getLocationOfStringLiteralByte(const StringLiteral *SL,<br>
>                                                 unsigned ByteNo) const;<br>
><br>
> -private:<br>
> +private:<br>
> +  void CheckArrayAccess(const ArraySubscriptExpr *ae);<br>
>   bool CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall);<br>
>   bool CheckBlockCall(NamedDecl *NDecl, CallExpr *TheCall);<br>
><br>
><br>
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=125640&r1=125639&r2=125640&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=125640&r1=125639&r2=125640&view=diff</a><br>

> ==============================================================================<br>
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)<br>
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Tue Feb 15 19:57:07 2011<br>
> @@ -3080,3 +3080,33 @@<br>
>     << TRange << Op->getSourceRange();<br>
>  }<br>
><br>
> +void Sema::CheckArrayAccess(const clang::ArraySubscriptExpr *ae) {<br>
> +  const DeclRefExpr *dr =<br>
> +    dyn_cast<DeclRefExpr>(ae->getBase()->IgnoreParenImpCasts());<br>
> +  if (!dr)<br>
> +    return;<br>
> +  const VarDecl *vd = cast<VarDecl>(dr->getDecl());<br>
> +  const ConstantArrayType *cat = Context.getAsConstantArrayType(vd->getType());<br>
> +  if (!cat)<br>
> +    return;<br>
> +  const Expr *idx = ae->getIdx();<br>
> +  if (idx->isValueDependent())<br>
> +    return;<br>
> +  llvm::APSInt result;<br>
> +  if (!idx->isIntegerConstantExpr(result, Context))<br>
> +    return;<br>
> +  unsigned kind = 2;<br>
> +  if (result.slt(0))<br>
> +    kind = /* precedes */ 0;<br>
> +  else {<br>
> +    const llvm::APInt &size = cat->getSize();<br>
> +    if (size.getBitWidth() > result.getBitWidth())<br>
> +      result = result.sext(size.getBitWidth());<br>
> +    if (result.sge(size))<br>
> +      kind = /* excedes */ 1;<br>
> +  }<br>
> +  if (kind < 2)<br>
> +    Diag(ae->getBase()->getLocEnd(), diag::warn_array_index_out_of_bounds)<br>
> +      << kind << idx->getSourceRange();<br>
> +}<br>
> +<br>
><br>
> Modified: cfe/trunk/lib/Sema/SemaExpr.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=125640&r1=125639&r2=125640&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=125640&r1=125639&r2=125640&view=diff</a><br>

> ==============================================================================<br>
> --- cfe/trunk/lib/Sema/SemaExpr.cpp (original)<br>
> +++ cfe/trunk/lib/Sema/SemaExpr.cpp Tue Feb 15 19:57:07 2011<br>
> @@ -294,6 +294,9 @@<br>
>   if (T.hasQualifiers())<br>
>     T = T.getUnqualifiedType();<br>
><br>
> +  if (const ArraySubscriptExpr *ae = dyn_cast<ArraySubscriptExpr>(E))<br>
> +    CheckArrayAccess(ae);<br>
> +<br>
>   E = ImplicitCastExpr::Create(Context, T, CK_LValueToRValue,<br>
>                                E, 0, VK_RValue);<br>
>  }<br>
> @@ -7242,6 +7245,11 @@<br>
>     Diag(UO->getOperatorLoc(), diag::note_indirection_through_null);<br>
>   }<br>
><br>
> +  // Check for trivial buffer overflows.<br>
> +  if (const ArraySubscriptExpr *ae<br>
> +      = dyn_cast<ArraySubscriptExpr>(LHS->IgnoreParenCasts()))<br>
> +    CheckArrayAccess(ae);<br>
> +<br>
>   // C99 6.5.16p3: The type of an assignment expression is the type of the<br>
>   // left operand unless the left operand has qualified type, in which case<br>
>   // it is the unqualified version of the type of the left operand.<br>
><br>
> Modified: cfe/trunk/test/Analysis/out-of-bounds.c<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/out-of-bounds.c?rev=125640&r1=125639&r2=125640&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/out-of-bounds.c?rev=125640&r1=125639&r2=125640&view=diff</a><br>

> ==============================================================================<br>
> --- cfe/trunk/test/Analysis/out-of-bounds.c (original)<br>
> +++ cfe/trunk/test/Analysis/out-of-bounds.c Tue Feb 15 19:57:07 2011<br>
> @@ -1,4 +1,4 @@<br>
> -// RUN: %clang_cc1 -analyze -analyzer-check-objc-mem -analyzer-check-buffer-overflows -verify %s<br>
> +// RUN: %clang_cc1 -Wno-array-bounds -analyze -analyzer-check-objc-mem -analyzer-check-buffer-overflows -verify %s<br>
><br>
>  // Tests doing an out-of-bounds access after the end of an array using:<br>
>  // - constant integer index<br>
><br>
> Added: cfe/trunk/test/Sema/array-bounds.c<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/array-bounds.c?rev=125640&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/array-bounds.c?rev=125640&view=auto</a><br>

> ==============================================================================<br>
> --- cfe/trunk/test/Sema/array-bounds.c (added)<br>
> +++ cfe/trunk/test/Sema/array-bounds.c Tue Feb 15 19:57:07 2011<br>
> @@ -0,0 +1,16 @@<br>
> +// RUN: %clang_cc1 -verify %s<br>
> +<br>
> +int foo() {<br>
> +  int x[2];<br>
> +  int y[2];<br>
> +  int *p = &y[2]; // no-warning<br>
> +  (void) sizeof(x[2]); // no-warning<br>
> +  y[2] = 2; // expected-warning{{array index excedes last array element}}<br>
> +  return x[2] +  // expected-warning{{array index excedes last array element}}<br>
> +         y[-1] + // expected-warning{{array index precedes first array element}}<br>
> +         x[sizeof(x)] +  // expected-warning{{array index excedes last array element}}<br>
> +         x[sizeof(x) / sizeof(x[0])] +  // expected-warning{{array index excedes last array element}}<br>
> +         x[sizeof(x) / sizeof(x[0]) - 1] + // no-warning<br>
> +         x[sizeof(x[2])]; // expected-warning{{array index excedes last array element}}<br>
> +}<br>
> +<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
><br>
<br>
</div></div>This breaks the Chrome build, where we have some code like this:<br>
<br>
template <bool extendArray><br>
void myFunc() {<br>
    int arr[3 + (extendArray ? 1 : 0)];<br>
<br>
    if (extendArray)<br>
        arr[3] = 42;<br>
}<br></blockquote><div><br></div><div>You can always explicitly specialize myFunc for true and false. I think it would make the resulting code strictly more clean. I'm kinda glad the warning fires on this type of construct.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">void f() {<br>
    myFunc<false>();<br>
}<br>
<br>
(The real code is here:<br>
<a href="http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp#L245" target="_blank">http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/filters/FEConvolveMatrix.cpp#L245</a>)<br>

<br>
In fact, it seems to warn also in a case like this:<br>
<br>
void f() {<br>
    int arr[42];<br>
    if (0)<br>
        arr[100] = 5;<br>
}<br></blockquote><div><br></div><div>Similarly, why write the code this way? Now, we could perhaps suppress the warning when the entire subscript expression comes from a macro expansion (the only conceivable way I see for this to be strongly intentional code), but I'd like to understand how often it happens in code.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">Would it be possible to make the warning a bit more conservative?<br></blockquote><div><br></div><div>The examples you gave are rather hard, they would involve proper control flow analysis. That doesn't belong in warnings.</div>
</div>