[clang-tidy] Add fsetpos argument checker

[DerWaschbar]

Summary

According to https://wiki.sei.cmu.edu/confluence/x/x9UxBQ
bugprone-fsetpos-argument-check is created.
The check finds fsetpos() function calls where the argument
is not initialized with fgetpos()

[Eugene.Zelenko]

Please rebase from master. Current version is based on 9.0.

clang-tools-extra/clang-tidy/bugprone/FsetposArgumentCheck.cpp

13

Unnecessary empty line.

23

Unnecessary empty line.

clang-tools-extra/docs/ReleaseNotes.rst

200

Please use double back-ticks to highlight language constructs.

clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst

7

Please use double back-ticks to highlight language constructs.

16

Please indent code.

38

Please add newline.

clang-tools-extra/docs/clang-tidy/checks/cert-fio44-c.rst

11

Please add newline.

[DerWaschbar]

• Removed unnecessary empty lines.
• Added double back-ticks to highlight language constructs.
• Indented code in documentation.

[DerWaschbar]

This comment was removed by DerWaschbar.

[DerWaschbar]

Rebased

[aaron.ballman]

Thank you for working on this check, I think it's useful functionality. One concern I have, though, is that it's not flow-sensitive and should probably be implemented as a clang static analyzer check instead of a clang-tidy check. For instance, consider these three plausible issues:

// This only sets the offset on one code path.
void func(FILE *fp) {
  fpos_t offset;
  if (condition) {
    // ... code
    if (0 != fgetpos(fp, &offset))
      return;
    // ... code
  } else {
   // ... code
  }
  fsetpos(fp, &offset);
}

// This doesn't check the failure from getting the position and sets the position regardless.
void func(FILE *fp) {
  fpos_t offset;
  fgetpos(fp, &offset);
  // ... code
  fsetpos(fp, &offset);
}

// This function accepts the offset from the caller but the caller passes an invalid offset.
void func(FILE *fp, const fpos_t *offset) {
  fsetpos(fp, offset);
}
void caller(FILE *fp) {
  fpos_t offset;
  func(fp, &offset);
}

Have you considered writing a static analyzer check so you can do data and control flow analysis to catch issues like these?

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

103

Can you reorder to below fio38-c?

clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst

5

The underlines here don't look like they are correct.

36

the CERT C Coding Standard rule

[DerWaschbar]

In D79437#2052109, @aaron.ballman wrote:

Thank you for working on this check, I think it's useful functionality. One concern I have, though, is that it's not flow-sensitive and should probably be implemented as a clang static analyzer check instead of a clang-tidy check. For instance, consider these three plausible issues:

// This only sets the offset on one code path.
void func(FILE *fp) {
  fpos_t offset;
  if (condition) {
    // ... code
    if (0 != fgetpos(fp, &offset))
      return;
    // ... code
  } else {
   // ... code
  }
  fsetpos(fp, &offset);
}

// This doesn't check the failure from getting the position and sets the position regardless.
void func(FILE *fp) {
  fpos_t offset;
  fgetpos(fp, &offset);
  // ... code
  fsetpos(fp, &offset);
}

// This function accepts the offset from the caller but the caller passes an invalid offset.
void func(FILE *fp, const fpos_t *offset) {
  fsetpos(fp, offset);
}
void caller(FILE *fp) {
  fpos_t offset;
  func(fp, &offset);
}

Have you considered writing a static analyzer check so you can do data and control flow analysis to catch issues like these?

I have noticed those issues too, but most likely the getter/setter will be in the same function body and we could measure fast how common is that issue in the wild. Also, this was my first introductory project for Clang and with that, I can rewrite this as a Static Analyzer project or start working on another Clang-Tidy project.

[aaron.ballman]

In D79437#2052704, @DerWaschbar wrote:

In D79437#2052109, @aaron.ballman wrote:

Have you considered writing a static analyzer check so you can do data and control flow analysis to catch issues like these?

I have noticed those issues too, but most likely the getter/setter will be in the same function body and we could measure fast how common is that issue in the wild.

That doesn't match my intuition, but if you have data, that would be excellent for helping to make a decision.

Also, this was my first introductory project for Clang and with that, I can rewrite this as a Static Analyzer project or start working on another Clang-Tidy project.

Welcome! I think this functionality is likely useful in here as a clang-tidy check, but I'd be curious to see data on whether it finds true or false positives in the wild to help judge that. My gut instinct is that to do this properly, we'll want it in the static analyzer, but perhaps the tidy check is good enough. I'd be curious to know if others have different thoughts though (pinging @alexfh for visibility).

[aaron.ballman]

btw, if you're interested in exploring a static analyzer solution for this, here's a recent review of an analyzer feature that's in the same general problem space: https://reviews.llvm.org/D80018

[whisperity]

clang-tools-extra/test/clang-tidy/checkers/bugprone-fsetpos-argument-check.cpp

1

This is a C, not a C++ file, the extension should show it. In addition, a similar test should be added that uses std::fgetpos() and std::fsetpos(), and shows the matching still works.

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "ForwardingReferenceOverloadCheck.h"
+#include "FsetposArgumentCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundingsCheck.h"
 #include "InfiniteLoopCheck.h"

         "bugprone-forward-declaration-namespace");
     CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
         "bugprone-forwarding-reference-overload");
+    CheckFactories.registerCheck<FsetposArgumentCheck>(
+        "bugprone-fsetpos-argument-check");
     CheckFactories.registerCheck<InaccurateEraseCheck>(
         "bugprone-inaccurate-erase");
     CheckFactories.registerCheck<IncorrectRoundingsCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   ForwardingReferenceOverloadCheck.cpp
+  FsetposArgumentCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundingsCheck.cpp
   InfiniteLoopCheck.cpp

clang-tools-extra/clang-tidy/bugprone/FsetposArgumentCheck.h

+//===--- FsetposArgumentCheck.h - clang-tidy ----------------------*- C -*-===//
+//
+// 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_FSETPOSARGUMENTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_FSETPOSARGUMENTCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds call of 'fsetpos' functions, which do not have file position indicator
+/// obtained from 'fgetpos'.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-fsetpos-argument-check.html
+class FsetposArgumentCheck : public ClangTidyCheck {
+public:
+  FsetposArgumentCheck(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_FSETPOSARGUMENTCHECK_H

clang-tools-extra/clang-tidy/bugprone/FsetposArgumentCheck.cpp

+//===--- FsetposArgumentCheck.cpp - clang-tidy ----------------------------===//
+//
+// 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 "FsetposArgumentCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <iostream>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void FsetposArgumentCheck::registerMatchers(MatchFinder *Finder) {
+  const auto Offset = declRefExpr(to(varDecl().bind("offset")));
+  const auto WithOffset = hasArgument(
+      1,
+      ignoringImpCasts(anyOf(unaryOperator(hasUnaryOperand(Offset)), Offset)));
+
+  const auto IsOffset = declRefExpr(to(varDecl(equalsBoundNode("offset"))));
+  const auto HasOffset = hasAnyArgument(ignoringImpCasts(
+      anyOf(unaryOperator(hasUnaryOperand(IsOffset)), IsOffset)));
+
+  const auto OffsetInWrongFunction = hasAncestor(functionDecl(
+      hasDescendant(callExpr(HasOffset, unless(callee(functionDecl(hasAnyName(
+                                            "::fgetpos", "::fsetpos")))))
+                        .bind("non-fpos"))));
+  const auto OffsetInFgetpos = hasAncestor(functionDecl(hasDescendant(
+      callExpr(HasOffset, callee(functionDecl(hasName("::fgetpos")))))));
+
+  Finder->addMatcher(
+      callExpr(WithOffset, callee(functionDecl(hasName("::fsetpos"))),
+               anyOf(OffsetInWrongFunction, unless(OffsetInFgetpos)))
+          .bind("fsetpos"),
+      this);
+}
+
+void FsetposArgumentCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<CallExpr>("fsetpos");
+
+  diag(MatchedDecl->getBeginLoc(),
+       "file position indicator should be obtained only from 'fgetpos'");
+
+  const auto *WrongFunction = Result.Nodes.getNodeAs<CallExpr>("non-fpos");
+  if (WrongFunction)
+    diag(WrongFunction->getBeginLoc(),
+         "fpos_t object should be passed as an argument only to 'fsetpos' or "
+         "'fgetpos' functions.",
+         DiagnosticIDs::Note);
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../bugprone/BadSignalToKillThreadCheck.h"
+#include "../bugprone/FsetposArgumentCheck.h"
 #include "../bugprone/ReservedIdentifierCheck.h"
 #include "../bugprone/SignedCharMisuseCheck.h"
 #include "../bugprone/SpuriouslyWakeUpFunctionsCheck.h"

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

clang-tools-extra/docs/ReleaseNotes.rst

   Finds non-const global variables as described in check I.2 of C++ Core
   Guidelines.
 
+- New :doc:`bugprone-fsetpos-argument-check
+  <clang-tidy/checks/bugprone-fsetpos-argument-check>` check.
+
+  Finds call of ``fsetpos`` functions, which does not have file position indicator
+  obtained from ``fgetpos``.
+
 - New :doc:`bugprone-misplaced-pointer-arithmetic-in-alloc
   <clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc>` check.
 

   :doc:`bugprone-reserved-identifier
   <clang-tidy/checks/bugprone-reserved-identifier>` was added.
 
+- New alias :doc:`cert-fio44-c
+  <clang-tidy/checks/cert-fio44-c>` to
+  :doc:`bugprone-fsetpos-argument-check
+  <clang-tidy/checks/bugprone-fsetpos-argument-check>` was added.
+
 - New alias :doc:`cert-str34-c
   <clang-tidy/checks/cert-str34-c>` to
   :doc:`bugprone-signed-char-misuse

clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst

+.. title:: clang-tidy - bugprone-fsetpos-argument-check
+
+bugprone-fsetpos-argument-check
+===============================
+
+Finds call of ``fsetpos`` functions, which does not have file position indicator
+obtained from ``fgetpos``.
+
+.. code-block:: c
+
+        #include <stdio.h>
+        #include <string.h>
+
+        int opener(FILE *file) {
+        int rc;
+        fpos_t offset;
+
+        memset(&offset, 0, sizeof(offset));
+
+        if (file == NULL) {
+            return -1;
+        }
+
+        /* Read in data from file */
+
+        rc = fsetpos(file, &offset);
+        if (rc != 0 ) {
+            return rc;
+        }
+
+        return 0;
+        }
+
+
+This check corresponds to the CERT C Coding Standard rule
+`FIO44-C. Only use values for fsetpos() that are returned from fgetpos()
+<https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152071>`_.

clang-tools-extra/docs/clang-tidy/checks/cert-fio44-c.rst

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

clang-tools-extra/docs/clang-tidy/checks/list.rst

    `bugprone-fold-init-type <bugprone-fold-init-type.html>`_,
    `bugprone-forward-declaration-namespace <bugprone-forward-declaration-namespace.html>`_,
    `bugprone-forwarding-reference-overload <bugprone-forwarding-reference-overload.html>`_,
+   `bugprone-fsetpos-argument-check <bugprone-fsetpos-argument-check>` _,
    `bugprone-inaccurate-erase <bugprone-inaccurate-erase.html>`_, "Yes"
    `bugprone-incorrect-roundings <bugprone-incorrect-roundings.html>`_,
    `bugprone-infinite-loop <bugprone-infinite-loop.html>`_,

    `cert-err09-cpp <cert-err09-cpp.html>`_, `misc-throw-by-value-catch-by-reference <misc-throw-by-value-catch-by-reference.html>`_,
    `cert-err61-cpp <cert-err61-cpp.html>`_, `misc-throw-by-value-catch-by-reference <misc-throw-by-value-catch-by-reference.html>`_,
    `cert-fio38-c <cert-fio38-c.html>`_, `misc-non-copyable-objects <misc-non-copyable-objects.html>`_,
+   `cert-fio44-c <cert-fio44-c.html>`_, `bugprone-fsetpos-argument-check <bugprone-fsetpos-argument-check.html>`_,
    `cert-msc30-c <cert-msc30-c.html>`_, `cert-msc50-cpp <cert-msc50-cpp.html>`_,
    `cert-msc32-c <cert-msc32-c.html>`_, `cert-msc51-cpp <cert-msc51-cpp.html>`_,
    `cert-oop11-cpp <cert-oop11-cpp.html>`_, `performance-move-constructor-init <performance-move-constructor-init.html>`_, "Yes"

clang-tools-extra/test/clang-tidy/checkers/bugprone-fsetpos-argument-check.cpp

+// RUN: %check_clang_tidy %s bugprone-fsetpos-argument-check %t
+
+typedef unsigned int size_t;
+class FILE;
+struct fpos_t {};
+int fsetpos(FILE *stream, const fpos_t *pos);
+int fgetpos(FILE *stream, fpos_t *pos);
+void *memset(void *ptr, int value, size_t num);
+
+//---------------------------------------------------------------------
+
+int opener1(FILE *file) {
+  int rc;
+  fpos_t offset;
+
+  if (file == nullptr ) {
+    return -1;
+  }
+
+  rc = fgetpos(file, &offset);
+  if (rc != 0 ) {
+    return rc;
+  }
+
+  /* Read in data from file */
+
+  rc = fsetpos(file, &offset);
+  if (rc != 0 ) {
+    return rc;
+  }
+
+  return 0;
+}
+
+//---------------------------------------------------------------------
+
+int opener2(FILE *file) {
+  int rc;
+  fpos_t offset;
+
+  memset(&offset, 0, sizeof(offset));
+
+  if (file == nullptr) {
+    return -1;
+  }
+
+  /* Read in data from file */
+
+  rc = fsetpos(file, &offset);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: file position indicator should be obtained only from 'fgetpos' [bugprone-fsetpos-argument-check]
+  if (rc != 0 ) {
+    return rc;
+  }
+
+  return 0;
+}
+
+
+//---------------------------------------------------------------------