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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
