[clang-tidy] Add io-functions-misused checker

[hgabii]

Summary

Add misc-io-functions-misused checker to warns for cases when the return value of certain standard iostream C functions (fgetc(), getc(), getchar(), fgetwc(), getwc(), getwchar(), istream::get()) return int value which is then incorrectly stored in char typed values.

[malcolm.parsons]

Why only the wide versions?
Even narrow versions return an int so that you can check for EOF.

[Eugene.Zelenko]

May be bugprone is better module then misc?

docs/ReleaseNotes.rst

63

Please add () to function names and enclose each of them in ``. Same in documentation.

[alexfh]

clang-tidy/misc/IoFunctionsMisusedCheck.cpp

33 ↗

(On Diff #131940)

matchesName can be pretty expensive and should generally be avoided. What set of types should be matched here? Can they all be listed here (using hasAnyName)?

43–46 ↗

(On Diff #131940)

It looks like the code doesn't need to distinguish between these two. They can be bound to the same ID then.

clang-tidy/misc/IoFunctionsMisusedCheck.h

19 ↗

(On Diff #131940)

s/checker/check/

docs/clang-tidy/checks/misc-io-functions-misused.rst

26–27 ↗

(On Diff #131940)

The formatting is off.

[hgabii]

Sorry, I forgot about my pull request.

I fixed the issues what are identified by reviewers :

• Check moved to bugprone module
• () and `` added
• matchesName replaced with hasAnyName

-Unnecessary IDs depracated

• clang-tidy/misc/IoFunctionsMisusedCheck.h:19: "s/checker/check/" => Fixed all places
• Formatting fixed

Only thing left is to match all IO functions, not just the wide versions.

[aaron.ballman]

clang-tidy/bugprone/BugproneTidyModule.cpp

84

This name reads a bit awkwardly because usually "misused" would come before "io functions". However, given that we've already said it's prone to bugs, I think "misuse" is somewhat implied. Perhaps bugprone-io-functions or `bugprone-misused-io-functions'?

However, I think this is actually checking an existing CERT rule and perhaps should be added to the CERT module as cert-fio34-c and perhaps that's the only module it should live in?

clang-tidy/bugprone/IoFunctionsMisusedCheck.cpp

25–31 ↗

(On Diff #153863)

You can combine these hasAnyName() calls into one given that they are otherwise identical matchers.

26 ↗

(On Diff #153863)

Why only the wide char versions and not the narrow char versions as well? For instance, those are problematic on systems where sizeof(int) == sizeof(char). See https://wiki.sei.cmu.edu/confluence/display/c/FIO34-C.+Distinguish+between+characters+read+from+a+file+and+EOF+or+WEOF for more details.

[alexfh]

(removing from my dashboard)

[hgabii]

I moved to bugprone module and renamed to bugprone-io-functions.
I added as a reference for cert-fio34-c.

[Eugene.Zelenko]

docs/clang-tidy/checks/bugprone-io-functions.rst

5

Please adjust length.

[aaron.ballman]

clang-tidy/bugprone/IoFunctionsCheck.cpp

33

::std::istream

48–50

This diagnostic confuses me, so perhaps you can explain the situation you want to diagnose a bit further.

FIO34-C is about situations where sizeof(char) == sizeof(int) and the call returns EOF/WEOF. In that case, there's no way to distinguish between an EOF/error and a valid character. Suggesting to explicitly cast to a character type doesn't add any safety to the code -- the user needs to insert calls to feof() or ferror() instead (to make it portable, anyway) and should store the character value in a sufficiently large integer type before doing the comparison against EOF.

Are you trying to catch a different kind of bug?

[hgabii]

clang-tidy/bugprone/IoFunctionsCheck.cpp

33

True. Fixed.

48–50

Yes, I want to diagnose this problem. Do you think I need to add suggestions into the warning message to use feof() or ferror()?

[aaron.ballman]

clang-tidy/bugprone/IoFunctionsCheck.cpp

48–50

I think there are two different situations to think about:

1. If the target architecture has sizeof(char) == sizeof(int), no amount of casting will help the user because the bit pattern for EOF is the same whether stored as an int or a char on that platform. There, the user really must use feof() and ferror(). This situation is theoretically even worse for the wide versions of the APIs because sizeof(wint_t) == sizeof(wchar_t) quite commonly, except that it would require a terrible choice in character sets for wchar_t by the implementation because Unicode makes guarantees about certain bit patterns not being a valid character (like 0xFFFF in UTF-16). I don't know that we have to worry about the wide-char versions at all in practice, but if you know of a platform where this is a legitimate concern, I'd love to hear about it.

2. If the target architecture does not have this identical-size-in-band-error-indicator problem, then it might make sense to diagnose the code for portability reasons or it might make sense to not diagnose at all because the user won't run into it. Might be worth making this an optional feature of the check.

Either way, I don't know that we'll be able to suggest fixes for the user in a diagnostic message of reasonable length. This might be a case where we want to tell the user about the danger and let the documentation tell them how to fix it. Maybe a diagnostic like: "the value for %select{EOF|WEOF}0 is indistinguishable from a valid character with the same bit pattern; consider explicit checks for error or end-of-file indicators"

clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundingsCheck.h"
 #include "IntegerDivisionCheck.h"
+#include "IoFunctionsCheck.h"
 #include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"

         "bugprone-incorrect-roundings");
     CheckFactories.registerCheck<IntegerDivisionCheck>(
         "bugprone-integer-division");
+    CheckFactories.registerCheck<IoFunctionsCheck>(
+        "bugprone-io-functions");
     CheckFactories.registerCheck<LambdaFunctionNameCheck>(
         "bugprone-lambda-function-name");
     CheckFactories.registerCheck<MacroParenthesesCheck>(

clang-tidy/bugprone/CMakeLists.txt

   InaccurateEraseCheck.cpp
   IncorrectRoundingsCheck.cpp
   IntegerDivisionCheck.cpp
+  IoFunctionsCheck.cpp
   LambdaFunctionNameCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp

clang-tidy/bugprone/IoFunctionsCheck.h

+//===--- IoFunctionsCheck.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_IO_FUNCTIONS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_IO_FUNCTIONS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// This check warns for cases when the return value of certain standard
+/// iostream C functions (fgetc(), getc(), getchar(), fgetwc(), getwc(),
+/// getwchar(), istream::get()) return int value which is then incorrectly
+/// stored in char typed values which can cause overflow at type conversion.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-io-functions.html
+class IoFunctionsCheck : public ClangTidyCheck {
+public:
+  IoFunctionsCheck(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_IO_FUNCTIONS_H

clang-tidy/bugprone/IoFunctionsCheck.cpp

+//===--- IoFunctionsCheck.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 "IoFunctionsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void IoFunctionsCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      implicitCastExpr(
+          hasImplicitDestinationType(isAnyCharacter()),
+          anyOf(has(callExpr(callee(
+                    functionDecl(hasAnyName("::getc", "::getchar", "::fgetc",
+                                            "::getwchar", "::getwc", "::fgetwc",
+                                            "::std::getc", "::std::getchar",
+                                            "::std::fgetc", "::std::getwchar",
+                                            "::std::getwc", "::std::fgetwc"))
+                        .bind("FuncDecl")))),
+                has(cxxMemberCallExpr(
+                    on(hasType(namedDecl(hasAnyName("std::istream")))),
+                    callee(cxxMethodDecl(hasName("get")).bind("DeclOfGet"))))),
+          hasCastKind(CK_IntegralCast))
+          .bind("ImplicitCast"),
+      this);
+}
+
+void IoFunctionsCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedImplicitCastToChar =
+      Result.Nodes.getNodeAs<ImplicitCastExpr>("ImplicitCast");
+  const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>("FuncDecl");
+  const auto *DeclGet = Result.Nodes.getNodeAs<CXXMethodDecl>("DeclOfGet");
+
+  auto Diag = diag(
+      MatchedImplicitCastToChar->getLocStart(),
+      "consider to cast the return value of %0 from type integer to type char, "
+      "possible loss of precision if an error has occurred or the end of file "
+      "has been reached, EOF and WEOF can be lost");
+
+  if (FuncDecl)
+    Diag << FuncDecl;
+  else
+    Diag << DeclGet;
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tidy/cert/CERTTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "../bugprone/IoFunctionsCheck.h"
 #include "../google/UnnamedNamespaceInHeaderCheck.h"
 #include "../misc/NewDeleteOverloadsCheck.h"
 #include "../misc/NonCopyableObjects.h"

     // FLP
     CheckFactories.registerCheck<FloatLoopCounter>("cert-flp30-c");
     // FIO
+    CheckFactories.registerCheck<bugprone::IoFunctionsCheck>("cert-fio34-c");
     CheckFactories.registerCheck<misc::NonCopyableObjectsCheck>("cert-fio38-c");
     // ERR
     CheckFactories.registerCheck<StrToNumCheck>("cert-err34-c");

clang-tidy/cert/CMakeLists.txt

   clangBasic
   clangLex
   clangTidy
+  clangTidyBugproneModule
   clangTidyGoogleModule
   clangTidyMiscModule
   clangTidyPerformanceModule

docs/ReleaseNotes.rst

   Finds functions which may throw an exception directly or indirectly, but they
   should not.
 
+- New `bugprone-io-functions
+  <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-io-functions.html>`_ check
+
+  Check if the ``fgetc()``, ``getc()``, ``getchat()``, ``fgetwc()``, 
+  ``getwc()``, ``getwchar()``, ``istream::get()`` standard iostream C functions 
+  return values incorrectly stored in char type value.
+
 - New :doc:`bugprone-parent-virtual-call
   <clang-tidy/checks/bugprone-parent-virtual-call>` check.
 

docs/clang-tidy/checks/bugprone-io-functions.rst

+.. title:: clang-tidy - bugprone-io-functions
+
+bugprone-io-functions
+=====================
+
+``cert-fio34-c`` redirects here as an alias for this check.
+
+This check warns for cases when the return value of certain standard iostream C 
+functions (``fgetc()``, ``getc()``, ``getchar()``, ``fgetwc()``, ``getwc()``, 
+``getwchar()``, ``istream::get()``) return int value which is then incorrectly 
+stored  in char typed values which can cause overflow at type conversion.
+This check is valid in C and C++.
+
+Checking for implicit cast from integer to char.
+
+.. code-block:: c++
+
+    char c;
+    c = std::getchar(); // Implicit cast from integer to char
+
+Check will warns for ``fgetc()``, ``getc()``, ``getchar()``, ``fgetwc()``, 
+``getwc()``, ``getwchar()`` functions from global or 'std' namespace and will 
+warn for ``get()`` function from 'istream' namespace.
+
+.. code-block:: c++
+
+    namespace ownnamespace { ... }
+    std::istream is = ...
+    char c;
+    c = is.get(); // Warning about incorrectly stored int value
+    c = ownnamespace::get(); // Won't warn
+
+Check will also warns for incorrectly used functions in parameter passing.
+
+.. code-block:: c++
+
+    void f(char c){ ... }
+    f(std::fgetwc( ... )); // Warning about passed int value instead of char
+
+This check corresponds to CERT C Coding Standard rule `FIO34-C. Distinguish 
+between characters read from a file and EOF or WEOF
+<https://wiki.sei.cmu.edu/confluence/display/c/FIO34-C.+Distinguish+between+characters+read+from+a+file+and+EOF+or+WEOF>`_.

docs/clang-tidy/checks/cert-fio34-c.rst

+.. title:: clang-tidy - cert-fio34-c
+.. meta::
+   :http-equiv=refresh: 5;URL=bugprone-io-functions.html
+
+cert-fio34-c
+============
+
+The cert-fio34-c check is an alias, please see
+`bugprone-io-functions <bugprone-io-functions.html>`_ for more
+information.

docs/clang-tidy/checks/list.rst

    bugprone-inaccurate-erase
    bugprone-incorrect-roundings
    bugprone-integer-division
+   bugprone-io-functions
    bugprone-lambda-function-name
    bugprone-macro-parentheses
    bugprone-macro-repeated-side-effects

    cert-err58-cpp
    cert-err60-cpp
    cert-err61-cpp (redirects to misc-throw-by-value-catch-by-reference) <cert-err61-cpp>
+   cert-fio34-c (redirects to bugprone-io-functions) <cert-fio34-c>
    cert-fio38-c (redirects to misc-non-copyable-objects) <cert-fio38-c>
    cert-flp30-c
    cert-msc30-c (redirects to cert-msc50-cpp) <cert-msc30-c>

test/clang-tidy/bugprone-io-functions.cpp

+// RUN: %check_clang_tidy %s bugprone-io-functions %t
+
+typedef int wint_t;
+typedef void *FILE;
+
+namespace std {
+int getchar();
+int getc(FILE);
+int fgetc(FILE);
+wint_t getwchar();
+wint_t getwc(FILE);
+wint_t fgetwc(FILE);
+class istream {
+public:
+  wint_t get();
+};
+} // namespace std
+
+int char_io_getchar() {
+  char c;
+  c = std::getchar();
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: consider to cast the return value of 'getchar' from type integer to type char, possible loss of precision if an error has occurred or the end of file has been reached, EOF and WEOF can be lost [bugprone-io-functions]
+  return c;
+}
+
+int char_io_getc(FILE *fp) {
+  char c;
+  c = std::getc(fp);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: consider to cast the return value of 'getc' from type integer to type char, possible loss of precision if an error has occurred or the end of file has been reached, EOF and WEOF can be lost [bugprone-io-functions]
+  return c;
+}
+
+int char_io_fgetc(FILE *fp) {
+  char c;
+  c = std::fgetc(fp);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: consider to cast the return value of 'fgetc' from type integer to type char, possible loss of precision if an error has occurred or the end of file has been reached, EOF and WEOF can be lost [bugprone-io-functions]
+  return c;
+}
+
+int char_io_getwchar() {
+  char c;
+  c = std::getwchar();
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: consider to cast the return value of 'getwchar' from type integer to type char, possible loss of precision if an error has occurred or the end of file has been reached, EOF and WEOF can be lost [bugprone-io-functions]
+  return c;
+}
+
+int char_io_getwc(FILE *fp) {
+  char c;
+  c = std::getwc(fp);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: consider to cast the return value of 'getwc' from type integer to type char, possible loss of precision if an error has occurred or the end of file has been reached, EOF and WEOF can be lost [bugprone-io-functions]
+  return c;
+}
+
+int char_io_fgetwc(FILE *fp) {
+  char c;
+  c = std::fgetwc(fp);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: consider to cast the return value of 'fgetwc' from type integer to type char, possible loss of precision if an error has occurred or the end of file has been reached, EOF and WEOF can be lost [bugprone-io-functions]
+  return c;
+}
+
+int char_io_get(std::istream &is) {
+  char c;
+  c = is.get();
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: consider to cast the return value of 'get' from type integer to type char, possible loss of precision if an error has occurred or the end of file has been reached, EOF and WEOF can be lost [bugprone-io-functions]
+  return c;
+}
+
+void char_type_in_argument(char c) {}
+
+void call_char_type_in_argument_with_getchar(FILE *fp) {
+  char_type_in_argument(std::fgetc(fp));
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: consider to cast the return value of 'fgetc' from type integer to type char, possible loss of precision if an error has occurred or the end of file has been reached, EOF and WEOF can be lost [bugprone-io-functions]
+}
+
+void call_char_type_in_argument_with_getwchar() {
+  char_type_in_argument(std::getwchar());
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: consider to cast the return value of 'getwchar' from type integer to type char, possible loss of precision if an error has occurred or the end of file has been reached, EOF and WEOF can be lost [bugprone-io-functions]
+}
+
+// Negatives
+int int_io_getchar() {
+  int x;
+  x = std::getchar();
+  return x;
+}
+
+int int_io_getc(FILE *fp) {
+  int x;
+  x = std::getc(fp);
+  return x;
+}
+
+int int_io_fgetc(FILE *fp) {
+  int x;
+  x = std::fgetc(fp);
+  return x;
+}
+
+int int_io_getwchar() {
+  int x;
+  x = std::getwchar();
+  return x;
+}
+
+int int_io_getwc(FILE *fp) {
+  int x;
+  x = std::getwc(fp);
+  return x;
+}
+
+int int_io_fgetwc(FILE *fp) {
+  int x;
+  x = std::fgetwc(fp);
+  return x;
+}
+
+int int_io_get(std::istream &is) {
+  int x;
+  x = is.get();
+  return x;
+}