[PATCH] D83717: [clang-tidy] Add check fo SEI CERT item ENV32-C

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

One of the concerns I have with this not being a flow-sensitive check is that most of the bad situations are not going to be caught by the clang-tidy version of the check. The CERT rules show contrived code examples, but the more frequent issue looks like:

  void cleanup(struct whatever *ptr) {
    assert(ptr); // This potentially calls abort()
    free(ptr->buffer);
    free(ptr);
  }
  
  void some_cleanup_func(void) {
    for (size_t idx = 0; idx < GlobalElementCount; ++idx) {
      cleanup(GlobalElement[idx]);
    }
  }
  
  void some_exit_handler(void) {
    ...
    some_cleanup_func();
    ...
  }

The fact that we're not looking through the call sites (even without cross-TU support) means the check isn't going to catch the most problematic cases. You could modify the called function collector to gather this a bit better, but you'd issue false positives in flow-sensitive situations like:

  void some_cleanup_func(void) {
    for (size_t idx = 0; idx < GlobalElementCount; ++idx) {
      struct whatever *ptr = GlobalElement[idx];
      if (ptr) {
        // Now we know abort() won't be called
        cleanup(ptr);
      }
    }
  }

Have you run this check over any large code bases to see if it currently catches any true positive diagnostics?



================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:101
+  diag(RegisterCall->getBeginLoc(),
+       "exit-handler potentially calls an exit function instead of terminating "
+       "normally with a return");
----------------
I know it was my suggestion originally, but I realize that's just describing the code not what's wrong with it. How about: `exit handler potentially terminates the program without running other exit handlers`?


================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:114
+  diag(RegisterCall->getSourceRange().getBegin(),
+       "exit-handler potentially calls a longjmp instead of terminating "
+       "normally with a return");
----------------
Similar suggestion here: `exit handler potentially calls 'longjmp' which may fail to run other exit handlers`?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst:7
+Finds functions registered by ``atexit`` and ``at_quick_exit`` that are calling
+exit functions ``_Exit``, ``exit``, ``quick_exit`` or ``longjmp``.
+
----------------
`terminate`?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst:9
+
+All exit handlers must return normally
+--------------------------------------
----------------
You should not copy and paste the text from the CERT standard here. There would be copyright questions from that, but also, the CERT standard is a wiki that gets updated with some regularity so these docs are likely to get stale anyway.

We usually handle this by paraphrasing a bit about what the rule is checking for, and then provide a link directly to the CERT rule for the user to get the details.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717