llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Manthan Singla (ThanSin02426) <details> <summary>Changes</summary> This check finds calls to sprintf where the first argument is a fixed-size array and suggests replacing them with snprintf. Fixes #<!-- -->40511 --- Full diff: https://github.com/llvm/llvm-project/pull/182823.diff 8 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp (+3) - (modified) clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt (+1) - (added) clang-tools-extra/clang-tidy/bugprone/SprintfToSnprintfCheck.cpp (+48) - (added) clang-tools-extra/clang-tidy/bugprone/SprintfToSnprintfCheck.h (+33) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6) - (added) clang-tools-extra/docs/clang-tidy/checks/bugprone/sprintf-to-snprintf.rst (+27) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) - (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/sprintf-to-snprintf.cpp (+16) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 4150442c25d61..46d58b984f97d 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -74,6 +74,7 @@ #include "SignedCharMisuseCheck.h" #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" +#include "SprintfToSnprintfCheck.h" #include "SpuriouslyWakeUpFunctionsCheck.h" #include "StandaloneEmptyCheck.h" #include "StdNamespaceModificationCheck.h" @@ -176,6 +177,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-incorrect-enable-if"); CheckFactories.registerCheck<IncorrectEnableSharedFromThisCheck>( "bugprone-incorrect-enable-shared-from-this"); + CheckFactories.registerCheck<SprintfToSnprintfCheck>( + "bugprone-sprintf-to-snprintf"); CheckFactories.registerCheck<UnintendedCharOstreamOutputCheck>( "bugprone-unintended-char-ostream-output"); CheckFactories.registerCheck<ReturnConstRefFromParameterCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index db1256d91d311..21f3c5f00f12f 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -37,6 +37,7 @@ add_clang_library(clangTidyBugproneModule STATIC IncorrectEnableIfCheck.cpp IncorrectEnableSharedFromThisCheck.cpp InvalidEnumDefaultInitializationCheck.cpp + SprintfToSnprintfCheck.cpp UnintendedCharOstreamOutputCheck.cpp ReturnConstRefFromParameterCheck.cpp SuspiciousStringviewDataUsageCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/SprintfToSnprintfCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SprintfToSnprintfCheck.cpp new file mode 100644 index 0000000000000..e357beb3e182e --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/SprintfToSnprintfCheck.cpp @@ -0,0 +1,48 @@ +//===----------------------------------------------------------------------===// +// +// 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 "SprintfToSnprintfCheck.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +void SprintfToSnprintfCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + callExpr( + callee(functionDecl(hasName("::sprintf"))), + hasArgument( + 0, ignoringParenImpCasts(declRefExpr(to( + varDecl(hasType(constantArrayType())).bind("buffer") + )).bind("arg0")) + ) + ).bind("call"), + this); +} + +void SprintfToSnprintfCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call"); + const auto *Buffer = Result.Nodes.getNodeAs<VarDecl>("buffer"); + + if (!Call || !Buffer) + return; + + StringRef BufferName = Buffer->getName(); + + auto Diag = diag(Call->getBeginLoc(), "use 'snprintf' instead of 'sprintf' for fixed-size character arrays"); + + SourceLocation FuncNameLoc = Call->getExprLoc(); + Diag << FixItHint::CreateReplacement(FuncNameLoc, "snprintf"); + + SourceLocation InsertLoc = Call->getArg(1)->getBeginLoc(); + std::string SizeArg = "sizeof(" + BufferName.str() + "), "; + Diag << FixItHint::CreateInsertion(InsertLoc, SizeArg); +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/SprintfToSnprintfCheck.h b/clang-tools-extra/clang-tidy/bugprone/SprintfToSnprintfCheck.h new file mode 100644 index 0000000000000..2a9b4dd3c3041 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/SprintfToSnprintfCheck.h @@ -0,0 +1,33 @@ +//===----------------------------------------------------------------------===// +// +// 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_SPRINTFTOSNPRINTFCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SPRINTFTOSNPRINTFCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +/// FIXME: Write a short description. +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/bugprone/sprintf-to-snprintf.html +class SprintfToSnprintfCheck : public ClangTidyCheck { +public: + SprintfToSnprintfCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SPRINTFTOSNPRINTFCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index e7437e62ee77d..23757aefb13e2 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -97,6 +97,12 @@ Improvements to clang-tidy New checks ^^^^^^^^^^ +- New :doc:`bugprone-sprintf-to-snprintf + <clang-tidy/checks/bugprone/sprintf-to-snprintf>` check. + + Finds calls to ``sprintf`` where the destination is a fixed-size character + array and suggests replacing them with the safer ``snprintf``. + - New :doc:`llvm-type-switch-case-types <clang-tidy/checks/llvm/type-switch-case-types>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/sprintf-to-snprintf.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/sprintf-to-snprintf.rst new file mode 100644 index 0000000000000..dc57e1736f3bd --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/sprintf-to-snprintf.rst @@ -0,0 +1,27 @@ +.. title:: clang-tidy - bugprone-sprintf-to-snprintf + +bugprone-sprintf-to-snprintf +============================ + +Finds calls to ``sprintf`` where the destination is a fixed-size character array and replaces them with the safer ``snprintf``. + +It's a common idiom to have a fixed-size buffer of characters allocated on the stack and then to ``printf`` into the buffer. This can easily lead to buffer overflows. This check recommends that the counted version of the function is used instead. + +Example +------- + +.. code-block:: c++ + + void f() { + char buff[80]; + sprintf(buff, "Hello, %s!\n", "world"); + } + +Becomes: + +.. code-block:: c++ + + void f() { + char buff[80]; + snprintf(buff, sizeof(buff), "Hello, %s!\n", "world"); + } \ No newline at end of file diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 0eabd9929dc39..0136a4849a075 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -144,6 +144,7 @@ Clang-Tidy Checks :doc:`bugprone-signed-char-misuse <bugprone/signed-char-misuse>`, :doc:`bugprone-sizeof-container <bugprone/sizeof-container>`, :doc:`bugprone-sizeof-expression <bugprone/sizeof-expression>`, + :doc:`bugprone-sprintf-to-snprintf <bugprone/sprintf-to-snprintf>`, "Yes" :doc:`bugprone-spuriously-wake-up-functions <bugprone/spuriously-wake-up-functions>`, :doc:`bugprone-standalone-empty <bugprone/standalone-empty>`, "Yes" :doc:`bugprone-std-namespace-modification <bugprone/std-namespace-modification>`, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sprintf-to-snprintf.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sprintf-to-snprintf.cpp new file mode 100644 index 0000000000000..f5d6ff5c59ee4 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sprintf-to-snprintf.cpp @@ -0,0 +1,16 @@ +// RUN: %check_clang_tidy %s bugprone-sprintf-to-snprintf %t + +extern "C" int sprintf(char *str, const char *format, ...); +extern "C" int snprintf(char *s, unsigned long n, const char *format, ...); + +void f() { + char buff[80]; + sprintf(buff, "Hello, %s!\n", "world"); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'snprintf' instead of 'sprintf' for fixed-size character arrays [bugprone-sprintf-to-snprintf] + // CHECK-FIXES: snprintf(buff, sizeof(buff), "Hello, %s!\n", "world"); +} + +void ignore_pointers(char* ptr) { + sprintf(ptr, "Hello"); + // Should not trigger because it's not a fixed-size array. +} \ No newline at end of file `````````` </details> https://github.com/llvm/llvm-project/pull/182823 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
