[PATCH] D79437: [clang-tidy] Add fsetpos argument checker

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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?



================
Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:103
     // FIO
+    CheckFactories.registerCheck<bugprone::FsetposArgumentCheck>("cert-fio44-c");
     CheckFactories.registerCheck<misc::NonCopyableObjectsCheck>("cert-fio38-c");
----------------
Can you reorder to below fio38-c?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst:4
+bugprone-fsetpos-argument-check
+========================
+
----------------
The underlines here don't look like they are correct.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-fsetpos-argument-check.rst:35
+
+This check corresponds to the CERT C++ Coding Standard rule
+`FIO44-C. Only use values for fsetpos() that are returned from fgetpos()
----------------
the CERT C Coding Standard rule


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79437/new/

https://reviews.llvm.org/D79437