cryptoad updated this revision to Diff 88047.
cryptoad added a comment.
Addressing first batch of comments.
https://reviews.llvm.org/D29839
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/IstreamOverflowCheck.cpp
clang-tidy/misc/IstreamOverflowCheck.h
clang-tidy/misc/MiscTidyModule.cpp
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-istream-overflow.rst
test/clang-tidy/misc-istream-overflow.cpp
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<typename _CharT, typename _Traits>
+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<typename _CharT> struct char_traits {};
+template<typename _CharT, typename _Traits = char_traits<_CharT> >
+ class basic_istream;
+typedef basic_istream<char> istream;
+ template<typename _CharT, typename _Traits>
+ basic_istream<_CharT, _Traits>&
+ operator>>(basic_istream<_CharT, _Traits>& __in, _CharT* __s)
+ { return __in; }
+template<typename _CharT, typename _Traits>
+ 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;
+ }
+}
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: 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: 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<ArgumentCommentCheck>("misc-argument-comment");
CheckFactories.registerCheck<AssertSideEffectCheck>(
"misc-assert-side-effect");
+ CheckFactories.registerCheck<IstreamOverflowCheck>(
+ "misc-istream-overflow");
CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>(
"misc-unconventional-assign-operator");
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<Expr>("Dest");
+ const auto *DestDecl = Result.Nodes.getNodeAs<Decl>("DestDecl");
+ const auto *Op = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("Op");
+ const auto *Body = Result.Nodes.getNodeAs<CompoundStmt>("Body");
+ const auto *IStream = Result.Nodes.getNodeAs<VarDecl>("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<Expr>("Arg");
+ WidthCall = Node.getNodeAs<CXXMemberCallExpr>("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<Expr>("Arg");
+ SetwCall = Node.getNodeAs<CXXConstructExpr>("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/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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
