[cfe-commits] r137980 - in /cfe/trunk: include/clang/Basic/Builtins.def include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/Sema/SemaChecking.cpp test/Sema/warn-strlcpycat-size.c

[Eli Friedman]

On Thu, Aug 18, 2011 at 1:55 PM, Ted Kremenek <kremenek at apple.com> wrote:
> Author: kremenek
> Date: Thu Aug 18 15:55:45 2011
> New Revision: 137980
>
> URL: http://llvm.org/viewvc/llvm-project?rev=137980&view=rev
> Log:
> Reapply r137903, but fix the definition of size_t in the test case to use __SIZE_TYPE__ (and hence be portable).
>
> Also, change the warning to -Wstrl-incorrect-size.
>
> Added:
>    cfe/trunk/test/Sema/warn-strlcpycat-size.c
> Modified:
>    cfe/trunk/include/clang/Basic/Builtins.def
>    cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>    cfe/trunk/include/clang/Sema/Sema.h
>    cfe/trunk/lib/Sema/SemaChecking.cpp
>
> Modified: cfe/trunk/include/clang/Basic/Builtins.def
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Builtins.def?rev=137980&r1=137979&r2=137980&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/Builtins.def (original)
> +++ cfe/trunk/include/clang/Basic/Builtins.def Thu Aug 18 15:55:45 2011
> @@ -664,6 +664,9 @@
>  // POSIX setjmp.h
>  LIBBUILTIN(_longjmp, "vJi",       "fr",    "setjmp.h", ALL_LANGUAGES)
>  LIBBUILTIN(siglongjmp, "vSJi",    "fr",    "setjmp.h", ALL_LANGUAGES)
> +// non-standard but very common
> +LIBBUILTIN(strlcpy, "zc*cC*z",    "f",     "string.h", ALL_LANGUAGES)
> +LIBBUILTIN(strlcat, "zc*cC*z",    "f",     "string.h", ALL_LANGUAGES)
>  //   id objc_msgSend(id, SEL, ...)
>  LIBBUILTIN(objc_msgSend, "GGH.",   "f",     "objc/message.h", OBJC_LANG)
>
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=137980&r1=137979&r2=137980&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Aug 18 15:55:45 2011
> @@ -278,6 +278,13 @@
>   "argument to 'sizeof' in %0 call is the same pointer type %1 as the "
>   "%select{destination|source}2; expected %3 or an explicit length">,
>   InGroup<DiagGroup<"sizeof-pointer-memaccess">>;
> +def warn_strlcpycat_wrong_size : Warning<
> +  "size argument in %0 call appears to be size of the source; expected the size of "
> +  "the destination">,
> +  DefaultIgnore,
> +  InGroup<DiagGroup<"strl-incorrect-size">>;
> +def note_strlcpycat_wrong_size : Note<
> +  "change size argument to be the size of the destination">;
>
>  /// main()
>  // static/inline main() are not errors in C, just in C++.
>
> Modified: cfe/trunk/include/clang/Sema/Sema.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=137980&r1=137979&r2=137980&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Sema/Sema.h (original)
> +++ cfe/trunk/include/clang/Sema/Sema.h Thu Aug 18 15:55:45 2011
> @@ -5973,6 +5973,9 @@
>   void CheckMemaccessArguments(const CallExpr *Call, CheckedMemoryFunction CMF,
>                                IdentifierInfo *FnName);
>
> +  void CheckStrlcpycatArguments(const CallExpr *Call,
> +                                IdentifierInfo *FnName);
> +
>   void CheckReturnStackAddr(Expr *RetValExp, QualType lhsType,
>                             SourceLocation ReturnLoc);
>   void CheckFloatComparison(SourceLocation loc, Expr* lex, Expr* rex);
>
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=137980&r1=137979&r2=137980&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Aug 18 15:55:45 2011
> @@ -319,7 +319,7 @@
>                           TheCall->getCallee()->getLocStart());
>   }
>
> -  // Memset/memcpy/memmove/memcmp handling
> +  // Builtin handling
>   int CMF = -1;
>   switch (FDecl->getBuiltinID()) {
>   case Builtin::BI__builtin_memset:
> @@ -339,6 +339,11 @@
>   case Builtin::BImemmove:
>     CMF = CMF_Memmove;
>     break;
> +
> +  case Builtin::BIstrlcpy:
> +  case Builtin::BIstrlcat:
> +    CheckStrlcpycatArguments(TheCall, FnInfo);
> +    break;
>
>   case Builtin::BI__builtin_memcmp:
>     CMF = CMF_Memcmp;
> @@ -359,6 +364,7 @@
>     break;
>   }
>
> +  // Memset/memcpy/memmove handling
>   if (CMF != -1)
>     CheckMemaccessArguments(TheCall, CheckedMemoryFunction(CMF), FnInfo);
>
> @@ -1990,6 +1996,95 @@
>   }
>  }
>
> +// A little helper routine: ignore addition and subtraction of integer literals.
> +// This intentionally does not ignore all integer constant expressions because
> +// we don't want to remove sizeof().
> +static const Expr *ignoreLiteralAdditions(const Expr *Ex, ASTContext &Ctx) {
> +  Ex = Ex->IgnoreParenCasts();
> +
> +  for (;;) {
> +    const BinaryOperator * BO = dyn_cast<BinaryOperator>(Ex);
> +    if (!BO || !BO->isAdditiveOp())
> +      break;
> +
> +    const Expr *RHS = BO->getRHS()->IgnoreParenCasts();
> +    const Expr *LHS = BO->getLHS()->IgnoreParenCasts();
> +
> +    if (isa<IntegerLiteral>(RHS))
> +      Ex = LHS;
> +    else if (isa<IntegerLiteral>(LHS))
> +      Ex = RHS;
> +    else
> +      break;
> +  }
> +
> +  return Ex;
> +}
> +
> +// Warn if the user has made the 'size' argument to strlcpy or strlcat
> +// be the size of the source, instead of the destination.
> +void Sema::CheckStrlcpycatArguments(const CallExpr *Call,
> +                                    IdentifierInfo *FnName) {
> +
> +  // Don't crash if the user has the wrong number of arguments
> +  if (Call->getNumArgs() != 3)
> +    return;
> +
> +  const Expr *SrcArg = ignoreLiteralAdditions(Call->getArg(1), Context);
> +  const Expr *SizeArg = ignoreLiteralAdditions(Call->getArg(2), Context);
> +  const Expr *CompareWithSrc = NULL;
> +
> +  // Look for 'strlcpy(dst, x, sizeof(x))'
> +  if (const Expr *Ex = getSizeOfExprArg(SizeArg))
> +    CompareWithSrc = Ex;
> +  else {
> +    // Look for 'strlcpy(dst, x, strlen(x))'
> +    if (const CallExpr *SizeCall = dyn_cast<CallExpr>(SizeArg)) {
> +      if (SizeCall->isBuiltinCall(Context) == Builtin::BIstrlen
> +          && SizeCall->getNumArgs() == 1)
> +        CompareWithSrc = ignoreLiteralAdditions(SizeCall->getArg(0), Context);
> +    }
> +  }

strlcpy(dst, x, strlen(foo)) is clearly wrong no matter what foo is;
should we warn unconditionally?

> +
> +  if (!CompareWithSrc)
> +    return;
> +
> +  // Determine if the argument to sizeof/strlen is equal to the source
> +  // argument.  In principle there's all kinds of things you could do
> +  // here, for instance creating an == expression and evaluating it with
> +  // EvaluateAsBooleanCondition, but this uses a more direct technique:
> +  const DeclRefExpr *SrcArgDRE = dyn_cast<DeclRefExpr>(SrcArg);
> +  if (!SrcArgDRE)
> +    return;
> +
> +  const DeclRefExpr *CompareWithSrcDRE = dyn_cast<DeclRefExpr>(CompareWithSrc);
> +  if (!CompareWithSrcDRE ||
> +      SrcArgDRE->getDecl() != CompareWithSrcDRE->getDecl())
> +    return;
> +
> +  const Expr *OriginalSizeArg = Call->getArg(2);
> +  Diag(CompareWithSrcDRE->getLocStart(), diag::warn_strlcpycat_wrong_size)
> +    << OriginalSizeArg->getSourceRange() << FnName;
> +
> +  // Output a FIXIT hint if the destination is an array (rather than a
> +  // pointer to an array).  This could be enhanced to handle some
> +  // pointers if we know the actual size, like if DstArg is 'array+2'
> +  // we could say 'sizeof(array)-2'.
> +  const Expr *DstArg = Call->getArg(0)->IgnoreParenImpCasts();
> +
> +  if (DstArg->getType()->isArrayType()) {
> +    llvm::SmallString<128> sizeString;
> +    llvm::raw_svector_ostream OS(sizeString);
> +    OS << "sizeof(";
> +    DstArg->printPretty(OS, Context, 0, Context.PrintingPolicy);
> +    OS << ")";
> +
> +    Diag(OriginalSizeArg->getLocStart(), diag::note_strlcpycat_wrong_size)
> +      << FixItHint::CreateReplacement(OriginalSizeArg->getSourceRange(),
> +                                      OS.str());
> +  }
> +}

I would say this fixit isn't a good idea, both because we should be
forcing the programmer to think about what they are doing here, and
because it isn't necessarily correct.  (sizeof(dest) isn't guaranteed
to be the actual size of the destination; suppose, for example, dest
is a flexible array.)

-Eli