Index: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp =================================================================== --- clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -48,6 +48,7 @@ #include "StringConstructorCheck.h" #include "StringIntegerAssignmentCheck.h" #include "StringLiteralWithEmbeddedNulCheck.h" +#include "StrlenInArrayIndexCheck.h" #include "SuspiciousEnumUsageCheck.h" #include "SuspiciousIncludeCheck.h" #include "SuspiciousMemsetUsageCheck.h" @@ -149,6 +150,8 @@ "bugprone-string-integer-assignment"); CheckFactories.registerCheck( "bugprone-string-literal-with-embedded-nul"); + CheckFactories.registerCheck( + "bugprone-strlen-in-array-index"); CheckFactories.registerCheck( "bugprone-suspicious-enum-usage"); CheckFactories.registerCheck( Index: clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt =================================================================== --- clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -43,6 +43,7 @@ StringConstructorCheck.cpp StringIntegerAssignmentCheck.cpp StringLiteralWithEmbeddedNulCheck.cpp + StrlenInArrayIndexCheck.cpp SuspiciousEnumUsageCheck.cpp SuspiciousIncludeCheck.cpp SuspiciousMemsetUsageCheck.cpp Index: clang-tools-extra/clang-tidy/bugprone/StrlenInArrayIndexCheck.h =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/bugprone/StrlenInArrayIndexCheck.h @@ -0,0 +1,35 @@ +//===--- StrlenInArrayIndex.h - clang-tidy *- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STRLENINARRAYINDEXCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STRLENINARRAYINDEXCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Checks for the usage of strlen() on an array within its own [] operator. +/// This can lead to write-outside-bounds error if used in the wrong context. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-strlen-in-array-index.html +class StrlenInArrayIndexCheck : public ClangTidyCheck { +public: + StrlenInArrayIndexCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STRLENINARRAYINDEX_H Index: clang-tools-extra/clang-tidy/bugprone/StrlenInArrayIndexCheck.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/bugprone/StrlenInArrayIndexCheck.cpp @@ -0,0 +1,45 @@ +//===--- StrlenInArrayIndex.cpp - clang-tidy -===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "StrlenInArrayIndexCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +void StrlenInArrayIndexCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + arraySubscriptExpr( + allOf(hasBase(implicitCastExpr(hasSourceExpression( + declRefExpr(to(varDecl().bind("buf")))))), + hasIndex(allOf( + hasDescendant(callExpr( + allOf(callee(functionDecl( + hasAnyName("::strlen", "::std::strlen", + "::wcslen", "::std::wcslen"))), + hasArgument(0, declRefExpr(to(varDecl( + equalsBoundNode("buf")))))))), + anyOf(hasDescendant(binaryOperator(hasOperatorName("-"))), + binaryOperator(hasOperatorName("-"))))))) + .bind("arrayExpr"), + this); +} + +void StrlenInArrayIndexCheck::check(const MatchFinder::MatchResult &Result) { + diag(Result.Nodes.getNodeAs("arrayExpr")->getBeginLoc(), + "strlen with subtraction could cause out-of bounds memory access " + "in array's own subscript expression."); +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang Index: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp =================================================================== --- clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp +++ clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp @@ -14,6 +14,7 @@ #include "../bugprone/SignalHandlerCheck.h" #include "../bugprone/SignedCharMisuseCheck.h" #include "../bugprone/SpuriouslyWakeUpFunctionsCheck.h" +#include "../bugprone/StrlenInArrayIndexCheck.h" #include "../bugprone/UnhandledSelfAssignmentCheck.h" #include "../google/UnnamedNamespaceInHeaderCheck.h" #include "../misc/NewDeleteOverloadsCheck.h" Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -117,6 +117,14 @@ Finds condition variables in nested ``if`` statements that were also checked in the outer ``if`` statement and were not changed. +- New :doc:`bugprone-strlen-in-array-index + ` check. + + Finds ``strlen`` function calls if they are used on an array within the + array's own index ``operator[]``. This could cause out-of-bounds memory access + in some special scenarios. + + - New :doc:`concurrency-mt-unsafe ` check. @@ -140,6 +148,14 @@ Flags functions with Cognitive Complexity metric exceeding the configured limit. +New check aliases +^^^^^^^^^^^^^^^^^ + +- New alias :doc:`cert-fio37-c + ` to + :doc:`bugprone-strlen-in-array-index + ` was added. + Changes in existing checks ^^^^^^^^^^^^^^^^^^^^^^^^^^ Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-strlen-in-array-index.rst =================================================================== --- /dev/null +++ clang-tools-extra/docs/clang-tidy/checks/bugprone-strlen-in-array-index.rst @@ -0,0 +1,21 @@ +.. title:: clang-tidy - bugprone-strlen-in-array-index + +bugprone-strlen-in-array-index +============================== + +Finds ``strlen`` function calls if they are used on an array within the array's +own index ``operator[]``. If there's a `\0` character in the array, ``strlen`` +won't return the correct length (this can happen with binary data). This can +cause access-outside-array-bounds error, if the array is indexed with ``strlen``. + +Example: + +.. code-block:: c++ + +enum { BUFFER_SIZE = 1024 }; + +void func(void) { + char buf[BUFFER_SIZE]; + fgets(buf, sizeof(buf), stdin) == NULL); // read "" or '\0' as first char + buf[strlen(buf) - 1] = '\0'; // error +} Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-strlen-in-array-index.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/bugprone-strlen-in-array-index.cpp @@ -0,0 +1,51 @@ +// RUN: %check_clang_tidy %s bugprone-strlen-in-array-index %t + +typedef void *FILE; + +extern FILE *stdin; +#define NULL ((char *)0) +extern unsigned int strlen(const char *str); +extern char *fgets(char *str, int num, FILE *stream); +extern const char *strchr(const char *str, int character); +extern char *strchr(char *str, int character); + +namespace std { +extern unsigned int strlen(const char *str); +} + +// Examples were copied from the cert-fio37-c issue (with some added calls): +// https://wiki.sei.cmu.edu/confluence/display/c/FIO37-C.+Do+not+assume+that+fgets%28%29+or+fgetws%28%29+returns+a+nonempty+string+when+successful + +void func() { + unsigned int BUFFER_SIZE = 1024; + char buf[BUFFER_SIZE]; + + if (fgets(buf, sizeof(buf), stdin) == NULL) { + /* Handle error */ + } + // buf's first character can be '\0', or buf can be empty! + buf[strlen(buf) - 1] = '\0'; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: strlen with subtraction could cause out-of bounds memory access in array's own subscript expression. + buf[std::strlen(buf) - 1] = '\0'; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: strlen with subtraction could cause out-of bounds memory access in array's own subscript expression. + buf[::strlen(buf) - 1] = '\0'; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: strlen with subtraction could cause out-of bounds memory access in array's own subscript expression. +} + +void func2() { + unsigned int BUFFER_SIZE = 1024; + char buf[BUFFER_SIZE]; + char *p; + + if (fgets(buf, sizeof(buf), stdin)) { + p = strchr(buf, '\n'); + if (p) { + *p = '\0'; + } + } else { + /* Handle error */ + } + + buf[strlen(buf)] = '\0'; // no-warning + buf[strlen(buf) + 1] = '\0'; // no-warning +}