diff options
| -rw-r--r-- | include/clang/Basic/DiagnosticSemaKinds.td | 12 | ||||
| -rw-r--r-- | lib/Sema/SemaChecking.cpp | 91 | ||||
| -rw-r--r-- | test/SemaCXX/warn-memset-bad-sizeof.cpp | 17 |
3 files changed, 88 insertions, 32 deletions
diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 12f5f30b35..b608a5d694 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -269,9 +269,15 @@ def warn_dyn_class_memaccess : Warning< InGroup<DiagGroup<"dynamic-class-memaccess">>; def note_bad_memaccess_silence : Note< "explicitly cast the pointer to silence this warning">; -def warn_sizeof_pointer : Warning< - "the argument to sizeof is pointer type %0, expected %1 to match " - "%select{first|second}2 argument to %3">; +def warn_sizeof_pointer_expr_memaccess : Warning< + "argument to 'sizeof' in %0 call is the same expression as the " + "%select{destination|source}1; did you mean to " + "%select{dereference it|remove the addressof|provide an explicit length}2?">, + InGroup<DiagGroup<"sizeof-pointer-memaccess">>; +def warn_sizeof_pointer_type_memaccess : Warning< + "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">>; /// main() // static/inline main() are not errors in C, just in C++. diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp index 945964fc35..81506bf571 100644 --- a/lib/Sema/SemaChecking.cpp +++ b/lib/Sema/SemaChecking.cpp @@ -1828,18 +1828,25 @@ static bool isDynamicClassType(QualType T) { return false; } -/// \brief If E is a sizeof expression, returns the expression's type in -/// OutType. -static bool sizeofExprType(const Expr* E, QualType *OutType) { +/// \brief If E is a sizeof expression returns the argument expression, +/// otherwise returns NULL. +static const Expr *getSizeOfExprArg(const Expr* E) { if (const UnaryExprOrTypeTraitExpr *SizeOf = - dyn_cast<UnaryExprOrTypeTraitExpr>(E)) { - if (SizeOf->getKind() != clang::UETT_SizeOf) - return false; + dyn_cast<UnaryExprOrTypeTraitExpr>(E)) + if (SizeOf->getKind() == clang::UETT_SizeOf && !SizeOf->isArgumentType()) + return SizeOf->getArgumentExpr()->IgnoreParenImpCasts(); - *OutType = SizeOf->getTypeOfArgument(); - return true; - } - return false; + return 0; +} + +/// \brief If E is a sizeof expression returns the argument type. +static QualType getSizeOfArgType(const Expr* E) { + if (const UnaryExprOrTypeTraitExpr *SizeOf = + dyn_cast<UnaryExprOrTypeTraitExpr>(E)) + if (SizeOf->getKind() == clang::UETT_SizeOf) + return SizeOf->getTypeOfArgument(); + + return QualType(); } /// \brief Check for dangerous or invalid arguments to memset(). @@ -1858,6 +1865,12 @@ void Sema::CheckMemsetcpymoveArguments(const CallExpr *Call, unsigned LastArg = FnName->isStr("memset")? 1 : 2; const Expr *LenExpr = Call->getArg(2)->IgnoreParenImpCasts(); + + // We have special checking when the length is a sizeof expression. + QualType SizeOfArgTy = getSizeOfArgType(LenExpr); + const Expr *SizeOfArg = getSizeOfExprArg(LenExpr); + llvm::FoldingSetNodeID SizeOfArgID; + for (unsigned ArgIdx = 0; ArgIdx != LastArg; ++ArgIdx) { const Expr *Dest = Call->getArg(ArgIdx)->IgnoreParenImpCasts(); SourceRange ArgRange = Call->getArg(ArgIdx)->getSourceRange(); @@ -1866,20 +1879,54 @@ void Sema::CheckMemsetcpymoveArguments(const CallExpr *Call, if (const PointerType *DestPtrTy = DestTy->getAs<PointerType>()) { QualType PointeeTy = DestPtrTy->getPointeeType(); - // Don't warn about void pointers or char pointers as both are often used - // for directly representing memory, regardless of its underlying type. - if (PointeeTy->isVoidType() || PointeeTy->isCharType()) + // Never warn about void type pointers. This can be used to suppress + // false positives. + if (PointeeTy->isVoidType()) continue; - // Catch "memset(p, 0, sizeof(p))" -- needs to be sizeof(*p). - QualType SizeofTy; - if (sizeofExprType(LenExpr, &SizeofTy) && - Context.typesAreCompatible(SizeofTy, DestTy)) { - // Note: This complains about sizeof(typeof(p)) as well. - SourceLocation loc = LenExpr->getSourceRange().getBegin(); - Diag(loc, diag::warn_sizeof_pointer) - << SizeofTy << PointeeTy << ArgIdx << FnName; - break; + // Catch "memset(p, 0, sizeof(p))" -- needs to be sizeof(*p). Do this by + // actually comparing the expressions for equality. Because computing the + // expression IDs can be expensive, we only do this if the diagnostic is + // enabled. + if (SizeOfArg && + Diags.getDiagnosticLevel(diag::warn_sizeof_pointer_expr_memaccess, + SizeOfArg->getExprLoc())) { + // We only compute IDs for expressions if the warning is enabled, and + // cache the sizeof arg's ID. + if (SizeOfArgID == llvm::FoldingSetNodeID()) + SizeOfArg->Profile(SizeOfArgID, Context, true); + llvm::FoldingSetNodeID DestID; + Dest->Profile(DestID, Context, true); + if (DestID == SizeOfArgID) { + unsigned ActionIdx = 0; // Default is to suggest dereferencing. + if (const UnaryOperator *UnaryOp = dyn_cast<UnaryOperator>(Dest)) + if (UnaryOp->getOpcode() == UO_AddrOf) + ActionIdx = 1; // If its an address-of operator, just remove it. + if (Context.getTypeSize(PointeeTy) == Context.getCharWidth()) + ActionIdx = 2; // If the pointee's size is sizeof(char), + // suggest an explicit length. + DiagRuntimeBehavior(SizeOfArg->getExprLoc(), Dest, + PDiag(diag::warn_sizeof_pointer_expr_memaccess) + << FnName << ArgIdx << ActionIdx + << Dest->getSourceRange() + << SizeOfArg->getSourceRange()); + break; + } + } + + // Also check for cases where the sizeof argument is the exact same + // type as the memory argument, and where it points to a user-defined + // record type. + if (SizeOfArgTy != QualType()) { + if (PointeeTy->isRecordType() && + Context.typesAreCompatible(SizeOfArgTy, DestTy)) { + DiagRuntimeBehavior(LenExpr->getExprLoc(), Dest, + PDiag(diag::warn_sizeof_pointer_type_memaccess) + << FnName << SizeOfArgTy << ArgIdx + << PointeeTy << Dest->getSourceRange() + << LenExpr->getSourceRange()); + break; + } } unsigned DiagID; diff --git a/test/SemaCXX/warn-memset-bad-sizeof.cpp b/test/SemaCXX/warn-memset-bad-sizeof.cpp index b334500ef5..90ac50472e 100644 --- a/test/SemaCXX/warn-memset-bad-sizeof.cpp +++ b/test/SemaCXX/warn-memset-bad-sizeof.cpp @@ -30,23 +30,26 @@ void f(Mat m, const Foo& const_foo, char *buffer) { char arr[5]; char* parr[5]; Foo foo; + char* heap_buffer = new char[42]; /* Should warn */ memset(&s, 0, sizeof(&s)); // \ - // expected-warning {{the argument to sizeof is pointer type 'S *', expected 'S' to match first argument to 'memset'}} + // expected-warning {{argument to 'sizeof' in 'memset' call is the same expression as the destination}} memset(ps, 0, sizeof(ps)); // \ - // expected-warning {{the argument to sizeof is pointer type 'S *', expected 'S' to match first argument to 'memset'}} + // expected-warning {{argument to 'sizeof' in 'memset' call is the same expression as the destination}} memset(ps2, 0, sizeof(ps2)); // \ - // expected-warning {{the argument to sizeof is pointer type 'PS' (aka 'S *'), expected 'S' to match first argument to 'memset'}} + // expected-warning {{argument to 'sizeof' in 'memset' call is the same expression as the destination}} memset(ps2, 0, sizeof(typeof(ps2))); // \ - // expected-warning {{the argument to sizeof is pointer type 'typeof (ps2)' (aka 'S *'), expected 'S' to match first argument to 'memset'}} + // expected-warning {{argument to 'sizeof' in 'memset' call is the same pointer type}} memset(ps2, 0, sizeof(PS)); // \ - // expected-warning {{the argument to sizeof is pointer type 'PS' (aka 'S *'), expected 'S' to match first argument to 'memset'}} + // expected-warning {{argument to 'sizeof' in 'memset' call is the same pointer type}} + memset(heap_buffer, 0, sizeof(heap_buffer)); // \ + // expected-warning {{argument to 'sizeof' in 'memset' call is the same expression as the destination}} memcpy(&s, 0, sizeof(&s)); // \ - // expected-warning {{the argument to sizeof is pointer type 'S *', expected 'S' to match first argument to 'memcpy'}} + // expected-warning {{argument to 'sizeof' in 'memcpy' call is the same expression as the destination}} memcpy(0, &s, sizeof(&s)); // \ - // expected-warning {{the argument to sizeof is pointer type 'S *', expected 'S' to match second argument to 'memcpy'}} + // expected-warning {{argument to 'sizeof' in 'memcpy' call is the same expression as the source}} /* Shouldn't warn */ memset((void*)&s, 0, sizeof(&s)); |