Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -3,6 +3,7 @@ add_clang_library(clangTidyMiscModule ArgumentCommentCheck.cpp AssertSideEffectCheck.cpp + IstreamOverflowCheck.cpp MisplacedConstCheck.cpp UnconventionalAssignOperatorCheck.cpp BoolPointerImplicitConversionCheck.cpp Index: clang-tidy/misc/IstreamOverflowCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/IstreamOverflowCheck.h @@ -0,0 +1,38 @@ +//===--- IstreamOverflowCheck.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_MISC_ISTREAM_OVERFLOW_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ISTREAM_OVERFLOW_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// Attempt to find the uses of istream::operator>> (and derived classes) to a +/// char array that do not set a width prior to use as this could lead to a +/// potential overflow. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-istream-overflow.html + +class IstreamOverflowCheck : public ClangTidyCheck { +public: + IstreamOverflowCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_ISTREAM_OVERFLOW_H Index: clang-tidy/misc/IstreamOverflowCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/IstreamOverflowCheck.cpp @@ -0,0 +1,134 @@ +//===--- IstreamOverflowCheck.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 "IstreamOverflowCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +void IstreamOverflowCheck::registerMatchers(MatchFinder *Finder) { + const auto DeclRefIStream = declRefExpr( + to(varDecl().bind("IStream")), + hasType(cxxRecordDecl(isSameOrDerivedFrom("::std::basic_istream")))); + const auto HasCharPType = + hasType(type(anyOf(pointerType(pointee(isAnyCharacter())), + arrayType(hasElementType(isAnyCharacter()))))); + const auto BoundBody = hasBody(compoundStmt().bind("Body")); + Finder->addMatcher( + cxxOperatorCallExpr( + hasOverloadedOperatorName(">>"), + argumentCountIs(2), + hasArgument(0, anyOf(DeclRefIStream, hasDescendant(DeclRefIStream))), + hasArgument(1, + declRefExpr(HasCharPType, + hasDeclaration( + decl().bind("DestDecl"))).bind("Dest")), + anyOf(hasAncestor(stmt(anyOf(forStmt(BoundBody), + cxxForRangeStmt(BoundBody), + whileStmt(BoundBody), + doStmt(BoundBody)))), + hasAncestor(functionDecl(BoundBody)))) + .bind("Op"), + this); +} + +void IstreamOverflowCheck::check(const MatchFinder::MatchResult &Result) { + auto& Context = *Result.Context; + const auto *Dest = Result.Nodes.getNodeAs("Dest"); + const auto *DestDecl = Result.Nodes.getNodeAs("DestDecl"); + const auto *Op = Result.Nodes.getNodeAs("Op"); + const auto *Body = Result.Nodes.getNodeAs("Body"); + const auto *IStream = Result.Nodes.getNodeAs("IStream"); + + const QualType ArrayType = Dest->IgnoreImpCasts()->getType(); + const ConstantArrayType *ConstType = + Context.getAsConstantArrayType(ArrayType); + llvm::APSInt ArraySize; + // If we are dealing with a constant array, get its size. + if (ConstType) + ArraySize = ConstType->getSize(); + + // We are looking for a width() call on the same istream the operator >> is + // used on. If we can determine its argument, store it. + const auto WidthMatches = match(findAll( + cxxMemberCallExpr( + on(declRefExpr(to(varDecl(equalsNode(IStream))))), + callee(cxxMethodDecl(hasName("width"))), + argumentCountIs(1), + hasArgument(0, expr().bind("Arg"))).bind("WidthCall")), + *Body, Context); + bool HasWidthCall = !WidthMatches.empty(); + llvm::APSInt WidthSize; + const CXXMemberCallExpr *WidthCall; + for (const auto& Node : WidthMatches) { + const auto *Arg = Node.getNodeAs("Arg"); + WidthCall = Node.getNodeAs("WidthCall"); + if (!Arg->isIntegerConstantExpr(WidthSize, Context)) { + llvm::errs() << "Couldn't get width() size.\n"; + } + } + + // We are looking for a call to std::setw, which itself calls a struct + // std::_Setw constructor. If we can determine its argument, store it. + const auto SetwMatches = match(findAll( + cxxConstructExpr( + hasDeclaration(cxxMethodDecl(hasName("_Setw"))), + argumentCountIs(1), + hasArgument(0, callExpr( + argumentCountIs(1), + hasArgument(0, expr().bind("Arg"))))).bind("SetwCall")), + *Op, Context); + bool HasSetwCall = !SetwMatches.empty(); + llvm::APSInt SetwSize; + const CXXConstructExpr *SetwCall; + for (const auto& Node : SetwMatches) { + const auto *Arg = Node.getNodeAs("Arg"); + SetwCall = Node.getNodeAs("SetwCall"); + if (!Arg->isIntegerConstantExpr(SetwSize, Context)) { + llvm::errs() << "Couldn't get std::setw() size.\n"; + } + } + + // TODO(kostyak): figure out the order, the last one takes precedence? + if (!HasWidthCall && !HasSetwCall) { + diag(Op->getLocStart(), + "istream::operator>> used without setting width, this could result " + "in a buffer overflow"); + } else if (ArraySize != 0) { + llvm::APSInt Width; + if (HasWidthCall && WidthSize != 0) + Width = WidthSize; + if (HasSetwCall && SetwSize != 0) + Width = SetwSize; + if (ArraySize.getZExtValue() < Width.getZExtValue()) { + diag(Op->getLocStart(), + "width set for istream::operator>> (%0) is greater than the " + "destination buffer capacity (%1), this could result in a buffer " + "overflow") + << Width.toString(10) << ArraySize.toString(10); + diag(DestDecl->getLocation(), "buffer declared here", + DiagnosticIDs::Note); + if (HasSetwCall) + diag(SetwCall->getLocation(), "std::setw called here", + DiagnosticIDs::Note); + if (HasWidthCall) + diag(WidthCall->getExprLoc(), "width called here", + DiagnosticIDs::Note); + } + } +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -20,6 +20,7 @@ #include "InaccurateEraseCheck.h" #include "IncorrectRoundings.h" #include "InefficientAlgorithmCheck.h" +#include "IstreamOverflowCheck.h" #include "MacroParenthesesCheck.h" #include "MacroRepeatedSideEffectsCheck.h" #include "MisplacedConstCheck.h" @@ -65,6 +66,8 @@ CheckFactories.registerCheck("misc-argument-comment"); CheckFactories.registerCheck( "misc-assert-side-effect"); + CheckFactories.registerCheck( + "misc-istream-overflow"); CheckFactories.registerCheck("misc-misplaced-const"); CheckFactories.registerCheck( "misc-unconventional-assign-operator"); Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -66,6 +66,7 @@ misc-inaccurate-erase misc-incorrect-roundings misc-inefficient-algorithm + misc-istream-overflow misc-macro-parentheses misc-macro-repeated-side-effects misc-misplaced-const Index: docs/clang-tidy/checks/misc-istream-overflow.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-istream-overflow.rst @@ -0,0 +1,46 @@ +.. title:: clang-tidy - misc-istream-overflow + +misc-istream-overflow +===================== + +This check finds calls to ``istream::operator>>`` (and derived classes) into a +character buffer, that haven't set previously a width. This could result in a +buffer overflow as the function will keep on reading until it reaches a space +or EOF. If it finds an operation setting the width of the stream, the check +will attempt to verify that the size fits the destination buffer. + +There are several ways to not have this problem surface: + +- call the ``width`` member function prior to using ``operator>>``, this will + ensure that only the given number of characters will be stored in the + destination buffer. Note that the width of the stream is reset to 0 after + each call to ``operator>>``, hence it must be set repeatedly if in a loop for + example. + +- use ``std::setw``, which in turns will set the width of the stream. + +- use an ``std::string`` as a destination instead of a character array, as this + will prevent any possibility of overflow. + +Given: + +.. code-block:: c++ + + std::istream is; + char s[8]; + +It will trigger on: + +.. code-block:: c++ + // The stream's width hasn't been set + is >> s; + +but not on: + +.. code-block:: c++ + // The stream's width has been set through width() + is.width(8); + is >> s; + + // The stream's width has been set through std::setw() + is >> std::setw(8) >> s; Index: test/clang-tidy/misc-istream-overflow.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-istream-overflow.cpp @@ -0,0 +1,90 @@ +// RUN: %check_clang_tidy %s misc-istream-overflow %t + +// Quick mock of std::istream and what we need to test our plug-in. +namespace std { +struct _Setw { int _M_n; }; +inline _Setw setw(int __n) { return { __n }; } +typedef long int streamsize; +template +class basic_istream { + public: + typedef basic_istream<_CharT, _Traits> __istream_type; + basic_istream(); + ~basic_istream(); + bool eof() const { return false; } + streamsize width() const { return _M_width; } + streamsize width(streamsize __wide) { + streamsize __old = _M_width; + _M_width = __wide; + return __old; + } + protected: + streamsize _M_width; +}; +template struct char_traits {}; +template > + class basic_istream; +typedef basic_istream istream; + template + basic_istream<_CharT, _Traits>& + operator>>(basic_istream<_CharT, _Traits>& __in, _CharT* __s) + { return __in; } +template + inline basic_istream<_CharT, _Traits>& + operator>>(basic_istream<_CharT, _Traits>& __is, _Setw __f) + { + __is.width(__f._M_n); + return __is; + } +} + +void bad_1(std::istream &is) { + char s[8]; + is >> s; +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: istream::operator>> used without setting width +} + +void bad_2(std::istream &is) { + char s[8]; + is.width(16); + is >> s; +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: width set for istream::operator>> (16) is greater than the destination buffer capacity (8) +} + +void bad_3(std::istream &is) { + char s[8]; + is >> std::setw(16) >> s; +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: width set for istream::operator>> (16) is greater than the destination buffer capacity (8) +} + +// The stream's width is reset to 0 at the end of operator>>, it is typically +// not sufficient to set it prior to a loop, but it should be set within. +void bad_3() { + std::istream is; + char s[8]; + is.width(8); + while (!is.eof()) { + is >> s; +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: istream::operator>> used without setting width + } +} + +void good_1(std::istream &is) { + char s[8]; + is.width(8); + is >> s; +} + +void good_2(std::istream &is) { + char s[8]; + is >> std::setw(8) >> s; +} + +void good_3() { + std::istream is; + char s[8]; + while (!is.eof()) { + is.width(8); + is >> s; + } +}