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

[Nathan James via Phabricator via llvm-commits]

njames93 added a comment.

It would be a good idea to emit notes to help explain the problem code, I'm thinking something along the lines of:

  warning: exit-handler potentially calls a jump function. Handlers should terminate by returning [cert-env32-c]
  note: exit-handler declared here
  note: call to a jump function declared here

The second note would require you to track the call site along with the FunctionDecl. 
I wouldn't go as far as trying to track the entire call stack when functions in exit handlers call functions that exit. While its possible, it could end up looking messy.



================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:26
+namespace {
+constexpr StringRef EF__Exit = "_Exit";
+constexpr StringRef EF_exit = "exit";
----------------
Listen to the clang-tidy warnings.


================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:72
+void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) {
+  // clang-format off
+  Finder->addMatcher(
----------------
Is there a reason  for disabling format here? How messy is the matcher code when formatted?


================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:78
+          anyOf(
+            hasName(RF_atexit),
+            hasName(RF_at_quick_exit)
----------------
Use `hasAnyName`


================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:120-122
+    if (SeenFunctions.count(Current))
+      continue;
+    SeenFunctions.insert(Current);
----------------
nit:
```
if (!SeenFunctions.insert(Current).second)
  continue;
```


================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:151
+    // Add called functions to the worklist.
+    std::copy(Collector.begin(), Collector.end(),
+              std::back_inserter(CalledFunctions));
----------------
Use `llvm::copy` instead of `std::copy`


================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h:18
+
+///
+/// Checker for SEI CERT rule ENV32-C
----------------
Unnecessary empty line


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:87
+  <clang-tidy/checks/cert-env32-c>` check.
+  Finds functions registered by ``atexit`` and ``at_quick_exit`` those are calling
+  exit functions ``_Exit``, ``exit``, ``quick_exit`` or ``longjmp``.
----------------
s/those/that


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst:114
+
+This compliant solution does not call longjmp()but instead returns from the exit handler normally:
+
----------------
Space after `longjmp() `


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717