[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
Chandler Carruth
chandlerc at google.com
Thu Feb 17 03:08:42 PST 2011
On Thu, Feb 17, 2011 at 2:56 AM, Hans Wennborg <hans at chromium.org> wrote:
> 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;
> }
>
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.
> 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;
> }
>
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.
> Would it be possible to make the warning a bit more conservative?
>
The examples you gave are rather hard, they would involve proper control
flow analysis. That doesn't belong in warnings.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110217/10048651/attachment.html>
More information about the cfe-commits
mailing list