[clang-tidy] New misc-istream-overflow check

[cryptoad]

Summary

This is a new check for a somewhat obscure istream::operator>> behavior, when
the destination is a character array. Characters will be copied to the
destination until a separator is found, with no regard for the size of the
destination. In order to prevent a potential overflow, one would have to
set the width property of the stream via width() or std::setw(), and keep in
mind that the width is reset to 0 after each operator>> call.

With this check, we attempt to assess that istream::operator>> is used
properly, setting a width prior to the call when needed, and if sizes can be
deduced, make sure they are consistent.

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

docs/clang-tidy/checks/misc-istream-overflow.rst

7

Please highlight language constructs with ``. Same below.

29

Please use .. code-block:: c++. See other checks documentation as example. Same below.

[Prazek]

Nice check! :)

clang-tidy/misc/IstreamOverflowCheck.cpp

60–62

same here

79–81

debug?

112–117

please remove unnecessary braces

126–133

same here

[cryptoad]

clang-tidy/misc/IstreamOverflowCheck.cpp

79–81

Oops!
I'd like to somehow keep that in there (for debug purposes). Any preferred way> (ifdef DEBUG or other).

[cryptoad]

Addressing first batch of comments.

[cryptoad]

Missing line separators.

[xazax.hun]

Shouldn't this be a path sensitive check within the clang static analyzer instead? So branches are properly handled and interprocedural analysis is done.

[Prazek]

In D29839#674301, @xazax.hun wrote:

Shouldn't this be a path sensitive check within the clang static analyzer instead? So branches are properly handled and interprocedural analysis is done.

Do you have some examples? I would argue, that even if you would have code that firstly uses width(), and then after a while reads input, then this is bugprone, and probably the line initializing width should be just before reading.

[xazax.hun]

In D29839#674307, @Prazek wrote:

In D29839#674301, @xazax.hun wrote:

Shouldn't this be a path sensitive check within the clang static analyzer instead? So branches are properly handled and interprocedural analysis is done.

Do you have some examples? I would argue, that even if you would have code that firstly uses width(), and then after a while reads input, then this is bugprone, and probably the line initializing width should be just before reading.

You are right, reasonable code sets the width right before reading the input. But do we only want to catch bugs in reasonable code?

[Prazek]

In D29839#674315, @xazax.hun wrote:

In D29839#674307, @Prazek wrote:

In D29839#674301, @xazax.hun wrote:

Shouldn't this be a path sensitive check within the clang static analyzer instead? So branches are properly handled and interprocedural analysis is done.

Do you have some examples? I would argue, that even if you would have code that firstly uses width(), and then after a while reads input, then this is bugprone, and probably the line initializing width should be just before reading.

You are right, reasonable code sets the width right before reading the input. But do we only want to catch bugs in reasonable code?

We will catch bugs in resonable and not resonable code. But because clang-tidy is a linter, we will also warn about cases that might be valid, but looks buggy, making code resonable.
We won't gonna have any false negatives (missed bugs), we only can have more false positives (warnings about correct code), but because it is linter, it totally make sense to warn about these cases.

[aaron.ballman]

In D29839#674301, @xazax.hun wrote:

Shouldn't this be a path sensitive check within the clang static analyzer instead? So branches are properly handled and interprocedural analysis is done.

I agree; I think this check should be part of the static analyzer because it is path sensitive if we want it to be particularly useful. As it stands now, it will catch trivial bugs, but by designing it as a clang-tidy check, it isn't easily extensible to catch the bigger bugs across procedures.

[alexfh]

In D29839#674517, @aaron.ballman wrote:

In D29839#674301, @xazax.hun wrote:

Shouldn't this be a path sensitive check within the clang static analyzer instead? So branches are properly handled and interprocedural analysis is done.

I agree; I think this check should be part of the static analyzer because it is path sensitive if we want it to be particularly useful. As it stands now, it will catch trivial bugs, but by designing it as a clang-tidy check, it isn't easily extensible to catch the bigger bugs across procedures.

I totally agree with Aaron and Gabor. This analysis can't be properly implemented without path sensitivity and I can imagine many valid situations where it will be too noisy (custom functions or stream manipulators that hide width setting, for example). Clang-tidy has a bunch of lint-style analyses, but when there is a more appropriate technology to implement a certain analysis, it should be preferred. It's all trade-offs, but here path sensitive analysis seems to be a much better tool for the job.

[cryptoad]

So if I understand correctly, the consensus is to abandon this and rewrite it to be part of the clang static analyzer?

[aaron.ballman]

In D29839#674714, @cryptoad wrote:

So if I understand correctly, the consensus is to abandon this and rewrite it to be part of the clang static analyzer?

That's correct.

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
+  IstreamOverflowCheck.cpp
   MisplacedConstCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   DanglingHandleCheck.cpp
   DefinitionsInHeadersCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   InaccurateEraseCheck.cpp

clang-tidy/misc/IstreamOverflowCheck.h

+//===--- 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

clang-tidy/misc/IstreamOverflowCheck.cpp

+//===--- 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

clang-tidy/misc/MiscTidyModule.cpp

 #include "BoolPointerImplicitConversionCheck.h"
 #include "DanglingHandleCheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
+#include "IstreamOverflowCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedConstCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "MoveForwardingReferenceCheck.h"
 #include "MultipleStatementMacroCheck.h"

 namespace misc {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     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");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "misc-bool-pointer-implicit-conversion");
     CheckFactories.registerCheck<DanglingHandleCheck>("misc-dangling-handle");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");

docs/clang-tidy/checks/list.rst

.. toctree::
    misc-bool-pointer-implicit-conversion
    misc-dangling-handle
    misc-definitions-in-headers
    misc-fold-init-type
    misc-forward-declaration-namespace
    misc-inaccurate-erase
    misc-incorrect-roundings
    misc-inefficient-algorithm
+   misc-istream-overflow
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-misplaced-const
    misc-misplaced-widening-cast
    misc-move-const-arg
    misc-move-constructor-init
    misc-move-forwarding-reference
    misc-multiple-statement-macro

docs/clang-tidy/checks/misc-istream-overflow.rst

+.. 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;

test/clang-tidy/misc-istream-overflow.cpp

+// 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;
+  }
+}