From 4c488975daf72e5ffd3b57971950d9e563d65074 Mon Sep 17 00:00:00 2001 From: Adam Balogh Date: Thu, 23 Nov 2017 12:33:12 +0000 Subject: [PATCH] [clang-tidy] Misplaced Operator in Strlen in Alloc A possible error is to write `malloc(strlen(s+1))` instead of `malloc(strlen(s)+1)`. Unfortunately the former is also valid syntactically, but allocates less memory by two bytes (if `s` is at least one character long, undefined behavior otherwise) which may result in overflow cases. This check detects such cases and also suggests the fix for them. Fix for r318906, forgot to add new files. llvm-svn: 318907 --- .../MisplacedOperatorInStrlenInAllocCheck.cpp | 92 ++++++++++++++++++++++ .../MisplacedOperatorInStrlenInAllocCheck.h | 37 +++++++++ ...prone-misplaced-operator-in-strlen-in-alloc.rst | 37 +++++++++ ...ugprone-misplaced-operator-in-strlen-in-alloc.c | 77 ++++++++++++++++++ ...prone-misplaced-operator-in-strlen-in-alloc.cpp | 33 ++++++++ 5 files changed, 276 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone-misplaced-operator-in-strlen-in-alloc.rst create mode 100644 clang-tools-extra/test/clang-tidy/bugprone-misplaced-operator-in-strlen-in-alloc.c create mode 100644 clang-tools-extra/test/clang-tidy/bugprone-misplaced-operator-in-strlen-in-alloc.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp new file mode 100644 index 0000000..2fee442 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp @@ -0,0 +1,92 @@ +//===--- MisplacedOperatorInStrlenInAllocCheck.cpp - clang-tidy------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "MisplacedOperatorInStrlenInAllocCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +void MisplacedOperatorInStrlenInAllocCheck::registerMatchers( + MatchFinder *Finder) { + const auto StrLenFunc = functionDecl(anyOf( + hasName("::strlen"), hasName("::std::strlen"), hasName("::strnlen"), + hasName("::std::strnlen"), hasName("::strnlen_s"), + hasName("::std::strnlen_s"), hasName("::wcslen"), + hasName("::std::wcslen"), hasName("::wcsnlen"), hasName("::std::wcsnlen"), + hasName("::wcsnlen_s"), hasName("std::wcsnlen_s"))); + + const auto BadUse = + callExpr(callee(StrLenFunc), + hasAnyArgument(ignoringImpCasts( + binaryOperator(allOf(hasOperatorName("+"), + hasRHS(ignoringParenImpCasts( + integerLiteral(equals(1)))))) + .bind("BinOp")))) + .bind("StrLen"); + + const auto BadArg = anyOf( + allOf(hasDescendant(BadUse), + unless(binaryOperator(allOf( + hasOperatorName("+"), hasLHS(BadUse), + hasRHS(ignoringParenImpCasts(integerLiteral(equals(1)))))))), + BadUse); + + const auto Alloc0Func = + functionDecl(anyOf(hasName("::malloc"), hasName("std::malloc"), + hasName("::alloca"), hasName("std::alloca"))); + const auto Alloc1Func = + functionDecl(anyOf(hasName("::calloc"), hasName("std::calloc"), + hasName("::realloc"), hasName("std::realloc"))); + + Finder->addMatcher( + callExpr(callee(Alloc0Func), hasArgument(0, BadArg)).bind("Alloc"), this); + Finder->addMatcher( + callExpr(callee(Alloc1Func), hasArgument(1, BadArg)).bind("Alloc"), this); +} + +void MisplacedOperatorInStrlenInAllocCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *Alloc = Result.Nodes.getNodeAs("Alloc"); + const auto *StrLen = Result.Nodes.getNodeAs("StrLen"); + const auto *BinOp = Result.Nodes.getNodeAs("BinOp"); + + const StringRef StrLenText = Lexer::getSourceText( + CharSourceRange::getTokenRange(StrLen->getSourceRange()), + *Result.SourceManager, getLangOpts()); + const StringRef StrLenBegin = StrLenText.substr(0, StrLenText.find('(') + 1); + const StringRef Arg0Text = Lexer::getSourceText( + CharSourceRange::getTokenRange(StrLen->getArg(0)->getSourceRange()), + *Result.SourceManager, getLangOpts()); + const StringRef StrLenEnd = StrLenText.substr( + StrLenText.find(Arg0Text) + Arg0Text.size(), StrLenText.size()); + + const StringRef LHSText = Lexer::getSourceText( + CharSourceRange::getTokenRange(BinOp->getLHS()->getSourceRange()), + *Result.SourceManager, getLangOpts()); + const StringRef RHSText = Lexer::getSourceText( + CharSourceRange::getTokenRange(BinOp->getRHS()->getSourceRange()), + *Result.SourceManager, getLangOpts()); + + auto Hint = FixItHint::CreateReplacement( + StrLen->getSourceRange(), + (StrLenBegin + LHSText + StrLenEnd + " + " + RHSText).str()); + + diag(Alloc->getLocStart(), + "addition operator is applied to the argument of %0 instead of its " + "result") << StrLen->getDirectCallee()->getName() << Hint; +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.h b/clang-tools-extra/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.h new file mode 100644 index 0000000..99cfcfb --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.h @@ -0,0 +1,37 @@ +//===--- MisplacedOperatorInStrlenInAllocCheck.h - clang-tidy----*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MISPLACED_OPERATOR_IN_STRLEN_IN_ALLOC_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MISPLACED_OPERATOR_IN_STRLEN_IN_ALLOC_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Finds cases where ``1`` is added to the string in the argument to a function +/// in the ``strlen()`` family instead of the result and value is used as an +/// argument to a memory allocation function. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-misplaced-operator-in-strlen-in-alloc.html +class MisplacedOperatorInStrlenInAllocCheck : public ClangTidyCheck { +public: + MisplacedOperatorInStrlenInAllocCheck(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_MISPLACED_OPERATOR_IN_STRLEN_IN_ALLOC_H diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-misplaced-operator-in-strlen-in-alloc.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-misplaced-operator-in-strlen-in-alloc.rst new file mode 100644 index 0000000..66b0308 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-misplaced-operator-in-strlen-in-alloc.rst @@ -0,0 +1,37 @@ +.. title:: clang-tidy - bugprone-misplaced-operator-in-strlen-in-alloc + +bugprone-misplaced-operator-in-strlen-in-alloc +============================================== + +Finds cases where ``1`` is added to the string in the argument to ``strlen()``, +``strnlen()``, ``strnlen_s()``, ``wcslen()``, ``wcsnlen()``, and ``wcsnlen_s()`` +instead of the result and the value is used as an argument to a memory +allocation function (``malloc()``, ``calloc()``, ``realloc()``, ``alloca()``). +Cases where ``1`` is added both to the parameter and the result of the +``strlen()``-like function are ignored, as are cases where the whole addition is +surrounded by extra parentheses. + +Example code: + +.. code-block:: c + + void bad_malloc(char *str) { + char *c = (char*) malloc(strlen(str + 1)); + } + + +The suggested fix is to add ``1`` to the return value of ``strlen()`` and not +to its argument. In the example above the fix would be + +.. code-block:: c + + char *c = (char*) malloc(strlen(str) + 1); + + +Example for silencing the diagnostic: + +.. code-block:: c + + void bad_malloc(char *str) { + char *c = (char*) malloc(strlen((str + 1))); + } diff --git a/clang-tools-extra/test/clang-tidy/bugprone-misplaced-operator-in-strlen-in-alloc.c b/clang-tools-extra/test/clang-tidy/bugprone-misplaced-operator-in-strlen-in-alloc.c new file mode 100644 index 0000000..b26bf24 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/bugprone-misplaced-operator-in-strlen-in-alloc.c @@ -0,0 +1,77 @@ +// RUN: %check_clang_tidy %s bugprone-misplaced-operator-in-strlen-in-alloc %t + +typedef __typeof(sizeof(int)) size_t; +void *malloc(size_t); +void *alloca(size_t); +void *calloc(size_t, size_t); +void *realloc(void *, size_t); + +size_t strlen(const char *); +size_t strnlen(const char *, size_t); +size_t strnlen_s(const char *, size_t); + +typedef unsigned wchar_t; + +size_t wcslen(const wchar_t *); +size_t wcsnlen(const wchar_t *, size_t); +size_t wcsnlen_s(const wchar_t *, size_t); + +void bad_malloc(char *name) { + char *new_name = (char *)malloc(strlen(name + 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: addition operator is applied to the argument of strlen + // CHECK-FIXES: {{^ char \*new_name = \(char \*\)malloc\(}}strlen(name) + 1{{\);$}} + new_name = (char *)malloc(strnlen(name + 1, 10)); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: addition operator is applied to the argument of strnlen + // CHECK-FIXES: {{^ new_name = \(char \*\)malloc\(}}strnlen(name, 10) + 1{{\);$}} + new_name = (char *)malloc(strnlen_s(name + 1, 10)); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: addition operator is applied to the argument of strnlen_s + // CHECK-FIXES: {{^ new_name = \(char \*\)malloc\(}}strnlen_s(name, 10) + 1{{\);$}} +} + +void bad_malloc_wide(wchar_t *name) { + wchar_t *new_name = (wchar_t *)malloc(wcslen(name + 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: addition operator is applied to the argument of wcslen + // CHECK-FIXES: {{^ wchar_t \*new_name = \(wchar_t \*\)malloc\(}}wcslen(name) + 1{{\);$}} + new_name = (wchar_t *)malloc(wcsnlen(name + 1, 10)); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: addition operator is applied to the argument of wcsnlen + // CHECK-FIXES: {{^ new_name = \(wchar_t \*\)malloc\(}}wcsnlen(name, 10) + 1{{\);$}} + new_name = (wchar_t *)malloc(wcsnlen_s(name + 1, 10)); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: addition operator is applied to the argument of wcsnlen_s + // CHECK-FIXES: {{^ new_name = \(wchar_t \*\)malloc\(}}wcsnlen_s(name, 10) + 1{{\);$}} +} + +void bad_alloca(char *name) { + char *new_name = (char *)alloca(strlen(name + 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: addition operator is applied to the argument of strlen + // CHECK-FIXES: {{^ char \*new_name = \(char \*\)alloca\(}}strlen(name) + 1{{\);$}} +} + +void bad_calloc(char *name) { + char *new_names = (char *)calloc(2, strlen(name + 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: addition operator is applied to the argument of strlen + // CHECK-FIXES: {{^ char \*new_names = \(char \*\)calloc\(2, }}strlen(name) + 1{{\);$}} +} + +void bad_realloc(char *old_name, char *name) { + char *new_name = (char *)realloc(old_name, strlen(name + 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: addition operator is applied to the argument of strlen + // CHECK-FIXES: {{^ char \*new_name = \(char \*\)realloc\(old_name, }}strlen(name) + 1{{\);$}} +} + +void intentional1(char *name) { + char *new_name = (char *)malloc(strlen(name + 1) + 1); + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:28: warning: addition operator is applied to the argument of strlen + // We have + 1 outside as well so we assume this is intentional +} + +void intentional2(char *name) { + char *new_name = (char *)malloc(strlen(name + 2)); + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:28: warning: addition operator is applied to the argument of strlen + // Only give warning for + 1, not + 2 +} + +void intentional3(char *name) { + char *new_name = (char *)malloc(strlen((name + 1))); + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:28: warning: addition operator is applied to the argument of strlen + // If expression is in extra parentheses, consider it as intentional +} diff --git a/clang-tools-extra/test/clang-tidy/bugprone-misplaced-operator-in-strlen-in-alloc.cpp b/clang-tools-extra/test/clang-tidy/bugprone-misplaced-operator-in-strlen-in-alloc.cpp new file mode 100644 index 0000000..406b043 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/bugprone-misplaced-operator-in-strlen-in-alloc.cpp @@ -0,0 +1,33 @@ +// RUN: %check_clang_tidy %s bugprone-misplaced-operator-in-strlen-in-alloc %t + +namespace std { +typedef __typeof(sizeof(int)) size_t; +void *malloc(size_t); + +size_t strlen(const char *); +} // namespace std + +namespace non_std { +typedef __typeof(sizeof(int)) size_t; +void *malloc(size_t); + +size_t strlen(const char *); +} // namespace non_std + +void bad_std_malloc_std_strlen(char *name) { + char *new_name = (char *)std::malloc(std::strlen(name + 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: addition operator is applied to the argument of strlen + // CHECK-FIXES: {{^ char \*new_name = \(char \*\)std::malloc\(}}std::strlen(name) + 1{{\);$}} +} + +void ignore_non_std_malloc_std_strlen(char *name) { + char *new_name = (char *)non_std::malloc(std::strlen(name + 1)); + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:28: warning: addition operator is applied to the argument of strlen + // Ignore functions of the malloc family in custom namespaces +} + +void ignore_std_malloc_non_std_strlen(char *name) { + char *new_name = (char *)std::malloc(non_std::strlen(name + 1)); + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:28: warning: addition operator is applied to the argument of strlen + // Ignore functions of the strlen family in custom namespaces +} -- 2.7.4