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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 abandoned this revision.
gamesh411 added a comment.

In D83717#2370154 , @NoQ wrote:

> I don't think you actually need active support for invoking exit handlers 
> path-sensitively at the end of `main()` in order to implement your checker. 
> You can still find out in a path-insensitive manner whether any given 
> function acts as an exit handler, like you already do. Then during 
> path-sensitive analysis of that function you can warn at any invocation of 
> exit().
>
> I do believe that you want to implement this check as a path-sensitive check 
> as long as you want any sort of interprocedural analysis (i.e., warn when an 
> exit handler calls a function that calls a function ... that calls a function 
> that calls `exit()` - and you do already have such tests). This is because of 
> the following potential situation:
>
>   void foo(bool already_exiting) {
> if (!already_exiting)
>   exit();
>   }
>   
>   void bar() {
> foo(true);
>   }
>   
>   void baz() {
> foo(false);
>   }
>   
>   int main() {
> atexit(bar);
> return 0;
>   }
>
> In this case `bar()` is an exit handler that calls `foo()` that calls 
> `exit()`. However the code is correct and no warning should be emitted, 
> because `foo()` would never call `exit()` //when called from// `bar()` 
> //specifically//. Precise reasoning about such problems requires 
> path-sensitive analysis. Of course it's up to you to decide what to do with 
> these false positives - whether you'll be ok with having them, or choose to 
> suppress with an imprecise heuristic - but that's one of the possible reasons 
> to consider reimplementing the checker via path sensitive analysis that the 
> static analyzer provides.
>
> We've had a similar problem with the checker that warns on calling pure 
> virtual functions in constructors/destructors (which is undefined behavior). 
> Such checker had to be path sensitive in order to be interprocedural for the 
> exact same unobvious reason. We've decided to re-implement it with 
> path-sensitive analysis and now we're pretty happy about that decision.

Thanks! I have checked, and this is definitely doable (however the solution is 
a bit more involved in case of CTU). This check is definitely better to 
implement in a path-sensitive way.

For anyone looking to implement this (thanks to @NoQ for the original idea 
outline above) :

   This is conceptually a 2 phase solution.
  - Phase 1: detect the functions used as exit_handlers. As an approximation, 
this could be done based on syntax (with an ASTMatcher), not unlike the one 
implemented in this revision. This can be done on the whole TU in any check 
callback, so the availability of this information is not an issue. In case of 
CTU the AST is built during the analysis itself, so theoretically we would want 
to recheck the whole TU every time we use this information (but this seems 
overkill, and computationally intensive). So doing this only once is also an 
approximation.
  - Phase2: during symbolic execution, we check for call expressions of exit 
functions (`_Exit`, `exit`, `quick_exit`), and examine the call-stack to 
identify a parent function declaration, which is in the set of detected 
handler-functions (built during Phase 1). We report an error in this case.

I guess I will abandon this and will create a patch for ClangSA when I will 
have some time for it (can't really promise anything as of now).


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

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I don't think you actually need active support for invoking exit handlers 
path-sensitively at the end of `main()` in order to implement your checker. You 
can still find out in a path-insensitive manner whether any given function acts 
as an exit handler, like you already do. Then during path-sensitive analysis of 
that function you can warn at any invocation of exit().

I do believe that you want to implement this check as a path-sensitive check as 
long as you want any sort of interprocedural analysis (i.e., warn when an exit 
handler calls a function that calls a function ... that calls a function that 
calls `exit()` - and you do already have such tests). This is because of the 
following potential situation:

  void foo(bool already_exiting) {
if (!already_exiting)
  exit();
  }
  
  void bar() {
foo(true);
  }
  
  void baz() {
foo(false);
  }
  
  int main() {
atexit(bar);
return 0;
  }

In this case `bar()` is an exit handler that calls `foo()` that calls `exit()`. 
However the code is correct and no warning should be emitted, because `foo()` 
would never call `exit()` //when called from// `bar()` //specifically//. 
Precise reasoning about such problems requires path-sensitive analysis. Of 
course it's up to you to decide what to do with these false positives - whether 
you'll be ok with having them, or choose to suppress with an imprecise 
heuristic - but that's one of the possible reasons to consider reimplementing 
the checker via path sensitive analysis that the static analyzer provides.

We've had a similar problem with the checker that warns on calling pure virtual 
functions in constructors/destructors (which is undefined behavior). Such 
checker had to be path sensitive in order to be interprocedural for the exact 
same unobvious reason. We've decided to re-implement it with path-sensitive 
analysis and now we're pretty happy about that decision.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: NoQ.
aaron.ballman added a subscriber: NoQ.
aaron.ballman added a comment.
Herald added a subscriber: Charusso.

In D83717#2366598 , @gamesh411 wrote:

>> Just to make sure we're on the same page -- the current approach is not 
>> flow-sensitive, and so my concern is that it won't report any true positives 
>> (not that it will be prone to false positives).
>
> Sorry about that. You are absolutely right; what I was trying to say is 
> CallGraph-based.
>
> Just some thoughts on this example:
>
> In D83717#2279263 , @aaron.ballman 
> wrote:
>
>> 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);
>>   }
>>   ...
>
> What I have in support of this approach is this reasoning:
> If a handler is used where either branch can abort then that branch is 
> expected to be taken. Otherwise it is dead code. I would argue then, that 
> this abortion should be refactored out of the handler function to ensure 
> well-defined behaviour in every possible case.

If the assert was directly within the handler code, then sure. However, 
consider a situation like this:

  struct Something {
Something(int *ip) { assert(ip); ... }
...
  };

where the use of the assertion is far removed from the fact that it's being 
used within a handler.

> As a counter-argument; suppose that there is a function that is used as both 
> an exit-handler and as a simple invocation. In this case, I can understand if 
> one would not want to factor the abortion logic out, or possibly pass flags 
> around.

Yes, this is exactly the situation I'm worried about.

> Then to this remark:
>
>> 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);
>>   }
>> }
>>   }
>
> The current approach definitely does not take 'adjacent' call-sites into 
> account (not to mention CTU ones).
> In this regard I also tend to see the benefit of this being a ClangSA checker 
> as that would solve 3 problems at once:
>
> 1. Being path-sensitive, so we can explain how we got to the erroneous 
> program-point
> 2. It utilizes CTU mode to take callsites from other TU-s into account
> 3. Runtime-stack building is implicitly done by ExprEngine as a side effect 
> of symbolic execution

Agreed.

> Counter-argument:
> But using ClangSA also introduces a big challenge.
> ClangSA analyzes all top-level functions during analysis. However I don't 
> know if it understands the concept of exit-handlers, and I don't know a way 
> of 'triggering' an analysis 'on-exit' so to speak.
> So AFAIK this model of analyzing only top-level functions is a limitation 
> when it comes to modelling the program behaviour 'on-exit'.

I'm hoping someone more well-versed in the details of the static analyzer can 
speak to this point. @NoQ @Szelethus others?

> sidenote:
> To validate this claim I have dumped the exploded graph of the following file:
>
>   #include 
>   #include 
>   
>   void f() {
> std::cout << "handler f";
>   };
>   
>   int main() {
> std::atexit(f);
>   }
>
> And it has no mention of std::cout being used, so I concluded, that ClangSA 
> does not model the 'on-exit' behaviour.
>
> I wanted to clear these issues before I made the documentation.
> Thanks for the effort and the tips on evaluating the solution, I will do some 
> more exploration.

Thank you for looking into this! If it turns out that there's some reason why 
the static analyzer cannot be at least as good of a home for the functionality 
as clang-tidy, that would be really interesting to learn. Either there are 
improvements we could consider making to the static analyzer, or we could leave 
the check in clang-tidy despite the limitations, but there's still a path 
forward.


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 added a comment.

> Just to make sure we're on the same page -- the current approach is not 
> flow-sensitive, and so my concern is that it won't report any true positives 
> (not that it will be prone to false positives).

Sorry about that. You are absolutely right; what I was trying to say is 
CallGraph-based.

Just some thoughts on this example:

In D83717#2279263 , @aaron.ballman 
wrote:

> 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);
>   }
>   ...

What I have in support of this approach is this reasoning:
If a handler is used where either branch can abort then that branch is expected 
to be taken. Otherwise it is dead code. I would argue then, that this abortion 
should be refactored out of the handler function to ensure well-defined 
behaviour in every possible case.

As a counter-argument; suppose that there is a function that is used as both an 
exit-handler and as a simple invocation. In this case, I can understand if one 
would not want to factor the abortion logic out, or possibly pass flags around.

Then to this remark:

> 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);
>   }
> }
>   }

The current approach definitely does not take 'adjacent' call-sites into 
account (not to mention CTU ones).
In this regard I also tend to see the benefit of this being a ClangSA checker 
as that would solve 3 problems at once:

1. Being path-sensitive, so we can explain how we got to the erroneous 
program-point
2. It utilizes CTU mode to take callsites from other TU-s into account
3. Runtime-stack building is implicitly done by ExprEngine as a side effect of 
symbolic execution

Counter-argument:
But using ClangSA also introduces a big challenge.
ClangSA analyzes all top-level functions during analysis. However I don't know 
if it understands the concept of exit-handlers, and I don't know a way of 
'triggering' an analysis 'on-exit' so to speak.
So AFAIK this model of analyzing only top-level functions is a limitation when 
it comes to modelling the program behaviour 'on-exit'.
sidenote:
To validate this claim I have dumped the exploded graph of the following file:

  #include 
  #include 
  
  void f() {
std::cout << "handler f";
  };
  
  int main() {
std::atexit(f);
  }

And it has no mention of std::cout being used, so I concluded, that ClangSA 
does not model the 'on-exit' behaviour.

I wanted to clear these issues before I made the documentation.
Thanks for the effort and the tips on evaluating the solution, I will do some 
more exploration.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D83717#2364187 , @gamesh411 wrote:

> In D83717#2279263 , @aaron.ballman 
> wrote:
>
>> One of the concerns I have with this not being a cfg-only check is that most 
>> of the bad situations are not going to be caught by the clang-tidy version 
>> of the check.
>
> ...
>
>> Have you run this check over any large code bases to see if it currently 
>> catches any true positive diagnostics?
>
> I have tried llvm, tmux, curl and tried codesearch.com to look for other 
> sources containing `atexit`, but no clang-tidy results were found for this 
> check (this is partly because it is hard to manually make the project results 
> buildable). So it is hard to see whether this flow-sensitive approach would 
> result in many false positives.

Just to make sure we're on the same page -- the current approach is not 
flow-sensitive and so my concern is that it won't report any true positives 
(not that it will be prone to false positives).

Btw, one trick I've used to make random projects easier to work with in 
clang-tidy is to either find ones that support CMake already or if they use 
make, then run the command through `bear` (https://github.com/rizsotto/Bear) -- 
this way I can get a compile_commands.json file that I can use to try to get 
clang-tidy diagnostics out of the project.

In any of the projects that you found which are using `atexit()`, did you try 
to inspect the exit handler code paths to see if you could manually identify 
any problem code (like `assert()` calls)? I realize that's a lot of effort to 
go through (especially if you have to consider C++ constructs like constructors 
or overloaded operators which may be hard to spot by code inspection), but if 
you find the check doesn't trigger on code bases but there are issues when 
manually inspecting the code, that strongly suggests this should be a 
flow-sensitive check that probably lives in the static analyzer rather than 
clang-tidy.


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 added a comment.

In D83717#2279263 , @aaron.ballman 
wrote:

> 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?

I have tried llvm, tmux, curl and tried codesearch.com to look for other 
sources containing `atexit`, but no results were found. So it is hard to see 
whether this flow-sensitive approach would result in many false positives.


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

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 292488.
gamesh411 added a comment.

Update commit message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c-teminate.cpp
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,412 @@
+// Test as a C file.
+// RUN: %check_clang_tidy %s cert-env32-c %t -- -- -DCMODE
+//
+// Test as a C++ file.
+//
+// Test functions in global namespace.
+// RUN: %check_clang_tidy -assume-filename=%s.cpp %s cert-env32-c %t \
+// RUN: -- -- -DCPPMODE
+//
+// Test functions in std namespace.
+// RUN: %check_clang_tidy -assume-filename=%s.cpp %s cert-env32-c %t \
+// RUN: -- -- -DCPPMODE -DTEST_NS_NAME=std
+
+#if defined(CPPMODE) && defined(TEST_NS_NAME)
+namespace TEST_NS_NAME {
+#endif
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// The program is terminated without destroying any object and without calling
+// any of the functions passed to atexit or at_quick_exit.
+void abort();
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function instead of terminating normally with a return [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-22]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-22]]:3: note: exit function called here
+  (void)at_quick_exit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function instead of terminating normally with a return [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-26]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-26]]:3: note: exit function called here
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function instead of terminating normally with a return [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-29]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-33]]:3: note: exit function called here
+  (void)at_quick_exit(call_call__Exit);
+  // 

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 292419.
gamesh411 added a comment.

Reformat diagnostic message
Use explicit name longjmp instead of jump function
Fix liberal auto inside Collector


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c-teminate.cpp
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,412 @@
+// Test as a C file.
+// RUN: %check_clang_tidy %s cert-env32-c %t -- -- -DCMODE
+//
+// Test as a C++ file.
+//
+// Test functions in global namespace.
+// RUN: %check_clang_tidy -assume-filename=%s.cpp %s cert-env32-c %t \
+// RUN: -- -- -DCPPMODE
+//
+// Test functions in std namespace.
+// RUN: %check_clang_tidy -assume-filename=%s.cpp %s cert-env32-c %t \
+// RUN: -- -- -DCPPMODE -DTEST_NS_NAME=std
+
+#if defined(CPPMODE) && defined(TEST_NS_NAME)
+namespace TEST_NS_NAME {
+#endif
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// The program is terminated without destroying any object and without calling
+// any of the functions passed to atexit or at_quick_exit.
+void abort();
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function instead of terminating normally with a return [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-22]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-22]]:3: note: exit function called here
+  (void)at_quick_exit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function instead of terminating normally with a return [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-26]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-26]]:3: note: exit function called here
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function instead of terminating normally with a return [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-29]]:1: note: handler function declared here
+  // CHECK-NOTES: 

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 marked 13 inline comments as done.
gamesh411 added a comment.

Note that there are no negative test cases that assert that we do NOT report in 
case a custom or an anonymous namespace is used. For that I would need a small 
patch in the testing infrastructure.
Patch needed in check_clang_tidy.py:

  --- a/clang-tools-extra/test/clang-tidy/check_clang_tidy.py
  +++ b/clang-tools-extra/test/clang-tidy/check_clang_tidy.py
  @@ -167,7 +167,7 @@ def run_test_once(args, extra_args):
 subprocess.check_output(
 ['FileCheck', '-input-file=' + temp_file_name, input_file_name,
  '-check-prefixes=' + ','.join(check_fixes_prefixes),
  -   '-strict-whitespace'],
  +   '-strict-whitespace', '--allow-empty'],
 stderr=subprocess.STDOUT)
   except subprocess.CalledProcessError as e:
 print('FileCheck failed:\n' + e.output.decode())
  @@ -180,7 +180,7 @@ def run_test_once(args, extra_args):
 subprocess.check_output(
 ['FileCheck', '-input-file=' + messages_file, input_file_name,
  '-check-prefixes=' + ','.join(check_messages_prefixes),
  -   '-implicit-check-not={{warning|error}}:'],
  +   '-implicit-check-not={{warning|error}}:', '--allow-empty'],
 stderr=subprocess.STDOUT)
   except subprocess.CalledProcessError as e:
 print('FileCheck failed:\n' + e.output.decode())
  @@ -195,7 +195,7 @@ def run_test_once(args, extra_args):
 subprocess.check_output(
 ['FileCheck', '-input-file=' + notes_file, input_file_name,
  '-check-prefixes=' + ','.join(check_notes_prefixes),
  -   '-implicit-check-not={{note|warning|error}}:'],
  +   '-implicit-check-not={{note|warning|error}}:', '--allow-empty'],
 stderr=subprocess.STDOUT)
   except subprocess.CalledProcessError as e:
 print('FileCheck failed:\n' + e.output.decode())

And then I can assert the non-reports by adding the following runlines:

  // Functions in anonymous or custom namespace should not be considered as exit
  // functions.
  //
  // RUN: %check_clang_tidy -assume-filename=%s.cpp %s -check-suffix=CUSTOM \
  // RUN: cert-env32-c %t -- -- -DCPPMODE -DTEST_NS_NAME=custom
  // CHECK-NOTES-CUSTOM-NOT: NO DIAGS
  //
  // RUN: %check_clang_tidy -assume-filename=%s.cpp %s -check-suffix=ANONYMOUS \
  // RUN: cert-env32-c %t -- -- -DCPPMODE -DTEST_NS_NAME=''
  // CHECK-NOTES-ANONYMOUS-NOT: NO DIAGS




Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:70
+ .bind("handler_expr"));
+  Finder->addMatcher(
+  callExpr(IsRegisterFunction, HasHandlerAsFirstArg).bind("register_call"),

aaron.ballman wrote:
> I am not at all certain whether this is plausible, but I *think* this check 
> can be almost entirely implemented in the matchers rather than having to do 
> manual work. I think you can bind the argument node from the call to 
> `at_quick_exit()` and then use `equalsBoundNode()` to find the function calls 
> within the bound `functionDecl()` node.
> 
> However, if that's not workable, I think you can get rid of the 
> `CalledFunctionsCollector` entirely and just use matchers directly within the 
> `check()` function because by that point, you'll know exactly which AST nodes 
> you want to traverse through.
I have investigated, and to do that recursive matching up to indeterminate 
depth all the while keeping the already matched functions in a set is not 
something I would implement with a single matcher. I could use a standalone 
matcher, but as far as I can understand I would need to implement a callback 
function for handling the matched results out of line for that as well, so I 
think that the ASTVisitor based solution is at least as good as a standalone 
ASTMatcher would be. Therefore I'd rather keep this solution.



Comment at: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c:31
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];

whisperity wrote:
> Which is the standard version this test file is set to analyse with? I don't 
> see any `-std=` flag in the `RUN:` line.
Right now there is a run-line for handling it as C, and 2 others for handling 
it as C++ with different namespaces. The test cases themselves are not 
dependent on the standard used (AFAIK).  The comment contains a standard 
reference for C, but that is just the first time it was standardized in C so 
the mention is there for traceability and not to restrict the standard used.


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 292416.
gamesh411 marked 2 inline comments as done.
gamesh411 added a comment.

Add abort and terminate handling
Extend tests to cover every exit functions
Extract matcher bind labels


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c-teminate.cpp
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,412 @@
+// Test as a C file.
+// RUN: %check_clang_tidy %s cert-env32-c %t -- -- -DCMODE
+//
+// Test as a C++ file.
+//
+// Test functions in global namespace.
+// RUN: %check_clang_tidy -assume-filename=%s.cpp %s cert-env32-c %t \
+// RUN: -- -- -DCPPMODE
+//
+// Test functions in std namespace.
+// RUN: %check_clang_tidy -assume-filename=%s.cpp %s cert-env32-c %t \
+// RUN: -- -- -DCPPMODE -DTEST_NS_NAME=std
+
+#if defined(CPPMODE) && defined(TEST_NS_NAME)
+namespace TEST_NS_NAME {
+#endif
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// The program is terminated without destroying any object and without calling
+// any of the functions passed to atexit or at_quick_exit.
+void abort();
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-22]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-22]]:3: note: exit function called here
+  (void)at_quick_exit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-26]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-26]]:3: note: exit function called here
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-29]]:1: note: handler function declared here
+  // 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:37
+return false;
+  constexpr StringRef ExitFunctions[] = {"_Exit", "exit", "quick_exit"};
+  return llvm::is_contained(ExitFunctions, FD->getName());

`abort()` as well?

How about in C++ mode calling `std::terminate()`?

One last class of problem functions are ones that are attributed as not 
returning (this would include user-defined functions).


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 289597.
gamesh411 added a comment.

only consider global and ::std scope handlers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,324 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-22]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-22]]:3: note: exit function called here
+  (void)at_quick_exit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-26]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-26]]:3: note: exit function called here
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-29]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-33]]:3: note: exit function called here
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-33]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-37]]:3: note: exit function called here
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-34]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-44]]:3: note: exit function called here
+  

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:64
+/// argument.
+void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) {
+  const auto IsRegisterFunction =

whisperity wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > aaron.ballman wrote:
> > > > whisperity wrote:
> > > > > What happens if this test is run on C++ code calling the same 
> > > > > functions? I see the rule is only applicable to C, for some reason... 
> > > > > Should it be disabled from registering if by accident the check is 
> > > > > enabled on a C++ source file?
> > > > The CERT C++ rules inherit many of the C rules, including this one. 
> > > > It's listed towards the bottom of the set of inherited rules here: 
> > > > https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=88046336
> > > Right, thanks for the heads-up. This should be somewhat more indicative 
> > > on the Wiki on the page for the rule (especially because people won't 
> > > immediately understand why a `-c` check reports on their cpp code, 
> > > assuming they understand `-c` means C.)
> > I would imagine people shouldn't be confused by a C check triggering in C++ 
> > given that C++ incorporates much of C so there's a considerable amount of 
> > overlap (for instance, this hasn't been an issue with `cert-env33-c` which 
> > applies in both C and C++).
> > 
> > That said, what do you think should be indicated on the wiki (I assume you 
> > mean the CERT wiki and not the clang-tidy documentation)? I'm happy to pass 
> > the suggestion along to folks I still know at CERT.
> > That said, what do you think should be indicated on the wiki (I assume you 
> > mean the CERT wiki and not the clang-tidy documentation)?
> 
> On the page of a C rule "ABC-00", if it also applies for C++, it should be 
> indicated. Just this fact: //"In addition, this rule is applicable for C++ 
> programs [bla bla]."// where `[bla bla]` is something like "calling C 
> standard library operations" or "dealing with C API" or "handling C data 
> structures" or simply no extra "elaboration", depending on what the rule is 
> targeting.
Thanks, I'll pass this suggestion along!


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

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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:64
+/// argument.
+void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) {
+  const auto IsRegisterFunction =

aaron.ballman wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > whisperity wrote:
> > > > What happens if this test is run on C++ code calling the same 
> > > > functions? I see the rule is only applicable to C, for some reason... 
> > > > Should it be disabled from registering if by accident the check is 
> > > > enabled on a C++ source file?
> > > The CERT C++ rules inherit many of the C rules, including this one. It's 
> > > listed towards the bottom of the set of inherited rules here: 
> > > https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=88046336
> > Right, thanks for the heads-up. This should be somewhat more indicative on 
> > the Wiki on the page for the rule (especially because people won't 
> > immediately understand why a `-c` check reports on their cpp code, assuming 
> > they understand `-c` means C.)
> I would imagine people shouldn't be confused by a C check triggering in C++ 
> given that C++ incorporates much of C so there's a considerable amount of 
> overlap (for instance, this hasn't been an issue with `cert-env33-c` which 
> applies in both C and C++).
> 
> That said, what do you think should be indicated on the wiki (I assume you 
> mean the CERT wiki and not the clang-tidy documentation)? I'm happy to pass 
> the suggestion along to folks I still know at CERT.
> That said, what do you think should be indicated on the wiki (I assume you 
> mean the CERT wiki and not the clang-tidy documentation)?

On the page of a C rule "ABC-00", if it also applies for C++, it should be 
indicated. Just this fact: //"In addition, this rule is applicable for C++ 
programs [bla bla]."// where `[bla bla]` is something like "calling C standard 
library operations" or "dealing with C API" or "handling C data structures" or 
simply no extra "elaboration", depending on what the rule is targeting.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:33
+bool isExitFunction(StringRef FnName) {
+  return FnName == "_Exit" || FnName == "exit" || FnName == "quick_exit";
+}

njames93 wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > Because this rule applies in C++ as well as C, you should protect these 
> > > names so that calling something like this doesn't trigger the check:
> > > ```
> > > namespace menu_items {
> > > void exit(int);
> > > }
> > > ```
> > > I think we want `::_Exit` and `::std::_Exit` to check the fully qualified 
> > > names (I believe this will still work in C, but should be tested). The 
> > > same goes for the other names (including `atexit` and `at_quick_exit` 
> > > above).
> > > I think we want `::_Exit` and `::std::_Exit` to check the fully qualified 
> > > names (I believe this will still work in C, but should be tested).
> > 
> > Tested with Clang-Query:
> > 
> > ```
> > clang-query> m functionDecl(hasName("::atexit"))
> > 
> > Match #1:
> > 
> > /home/whisperity/LLVM/Build/foo.c:1:1: note: "root" binds here
> > int atexit(int) {}
> > ^~
> > 1 match.
> > ```
> That only works because the logic inside the `hasName`matcher 
That's the bit I was worried about, too, thanks for confirming @njames93. 



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:64
+/// argument.
+void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) {
+  const auto IsRegisterFunction =

whisperity wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > What happens if this test is run on C++ code calling the same functions? 
> > > I see the rule is only applicable to C, for some reason... Should it be 
> > > disabled from registering if by accident the check is enabled on a C++ 
> > > source file?
> > The CERT C++ rules inherit many of the C rules, including this one. It's 
> > listed towards the bottom of the set of inherited rules here: 
> > https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=88046336
> Right, thanks for the heads-up. This should be somewhat more indicative on 
> the Wiki on the page for the rule (especially because people won't 
> immediately understand why a `-c` check reports on their cpp code, assuming 
> they understand `-c` means C.)
I would imagine people shouldn't be confused by a C check triggering in C++ 
given that C++ incorporates much of C so there's a considerable amount of 
overlap (for instance, this hasn't been an issue with `cert-env33-c` which 
applies in both C and C++).

That said, what do you think should be indicated on the wiki (I assume you mean 
the CERT wiki and not the clang-tidy documentation)? I'm happy to pass the 
suggestion along to folks I still know at CERT.


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 282837.
gamesh411 marked an inline comment as done.
gamesh411 added a comment.

rename file name in header


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,324 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-22]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-22]]:3: note: exit function called here
+  (void)at_quick_exit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-26]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-26]]:3: note: exit function called here
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-29]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-33]]:3: note: exit function called here
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-33]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-37]]:3: note: exit function called here
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-34]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-44]]:3: note: exit 

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

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:33
+bool isExitFunction(StringRef FnName) {
+  return FnName == "_Exit" || FnName == "exit" || FnName == "quick_exit";
+}

whisperity wrote:
> aaron.ballman wrote:
> > Because this rule applies in C++ as well as C, you should protect these 
> > names so that calling something like this doesn't trigger the check:
> > ```
> > namespace menu_items {
> > void exit(int);
> > }
> > ```
> > I think we want `::_Exit` and `::std::_Exit` to check the fully qualified 
> > names (I believe this will still work in C, but should be tested). The same 
> > goes for the other names (including `atexit` and `at_quick_exit` above).
> > I think we want `::_Exit` and `::std::_Exit` to check the fully qualified 
> > names (I believe this will still work in C, but should be tested).
> 
> Tested with Clang-Query:
> 
> ```
> clang-query> m functionDecl(hasName("::atexit"))
> 
> Match #1:
> 
> /home/whisperity/LLVM/Build/foo.c:1:1: note: "root" binds here
> int atexit(int) {}
> ^~
> 1 match.
> ```
That only works because the logic inside the `hasName`matcher 



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:32
 /// 'quick_exit'
 bool isExitFunction(StringRef FnName) {
   return FnName == "_Exit" || FnName == "exit" || FnName == "quick_exit";

This should take a pointer to the function to run other checks, like checking 
its decl context. 



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:37
 /// Only 'longjmp' is considered.
 bool isJumpFunction(StringRef FnName) { return FnName == "longjmp"; }
 

Ditto


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

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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:33
+bool isExitFunction(StringRef FnName) {
+  return FnName == "_Exit" || FnName == "exit" || FnName == "quick_exit";
+}

aaron.ballman wrote:
> Because this rule applies in C++ as well as C, you should protect these names 
> so that calling something like this doesn't trigger the check:
> ```
> namespace menu_items {
> void exit(int);
> }
> ```
> I think we want `::_Exit` and `::std::_Exit` to check the fully qualified 
> names (I believe this will still work in C, but should be tested). The same 
> goes for the other names (including `atexit` and `at_quick_exit` above).
> I think we want `::_Exit` and `::std::_Exit` to check the fully qualified 
> names (I believe this will still work in C, but should be tested).

Tested with Clang-Query:

```
clang-query> m functionDecl(hasName("::atexit"))

Match #1:

/home/whisperity/LLVM/Build/foo.c:1:1: note: "root" binds here
int atexit(int) {}
^~
1 match.
```



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:64
+/// argument.
+void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) {
+  const auto IsRegisterFunction =

aaron.ballman wrote:
> whisperity wrote:
> > What happens if this test is run on C++ code calling the same functions? I 
> > see the rule is only applicable to C, for some reason... Should it be 
> > disabled from registering if by accident the check is enabled on a C++ 
> > source file?
> The CERT C++ rules inherit many of the C rules, including this one. It's 
> listed towards the bottom of the set of inherited rules here: 
> https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=88046336
Right, thanks for the heads-up. This should be somewhat more indicative on the 
Wiki on the page for the rule (especially because people won't immediately 
understand why a `-c` check reports on their cpp code, assuming they understand 
`-c` means C.)



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:124-125
+  diag(RegisterCall->getSourceRange().getBegin(),
+   "exit-handler potentially calls a jump function. Handlers should "
+   "terminate by returning");
+  diag(HandlerDecl->getBeginLoc(), "handler function declared here",

aaron.ballman wrote:
> whisperity wrote:
> > This semi-sentence structure of starting with lowercase but also 
> > terminating the sentence and leaving in another but unterminated sentences 
> > looks really odd.
> > 
> > Suggestion: "exit handler potentially jumps instead of terminating normally 
> > with a return"
> Slight tweak here as well. I don't think we'll ever see a jump function other 
> than longjmp for this rule, so what about writing `potentially calls 
> 'longjmp'` instead of `potentially jumps`?
 Agree. And if we see, we will have to update the code anyways. One could in 
theory whip up some inline assembly black magic, but that's a whole other can 
of worms.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:33
+bool isExitFunction(StringRef FnName) {
+  return FnName == "_Exit" || FnName == "exit" || FnName == "quick_exit";
+}

Because this rule applies in C++ as well as C, you should protect these names 
so that calling something like this doesn't trigger the check:
```
namespace menu_items {
void exit(int);
}
```
I think we want `::_Exit` and `::std::_Exit` to check the fully qualified names 
(I believe this will still work in C, but should be tested). The same goes for 
the other names (including `atexit` and `at_quick_exit` above).



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:64
+/// argument.
+void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) {
+  const auto IsRegisterFunction =

whisperity wrote:
> What happens if this test is run on C++ code calling the same functions? I 
> see the rule is only applicable to C, for some reason... Should it be 
> disabled from registering if by accident the check is enabled on a C++ source 
> file?
The CERT C++ rules inherit many of the C rules, including this one. It's listed 
towards the bottom of the set of inherited rules here: 
https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=88046336



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:70
+ .bind("handler_expr"));
+  Finder->addMatcher(
+  callExpr(IsRegisterFunction, HasHandlerAsFirstArg).bind("register_call"),

I am not at all certain whether this is plausible, but I *think* this check can 
be almost entirely implemented in the matchers rather than having to do manual 
work. I think you can bind the argument node from the call to `at_quick_exit()` 
and then use `equalsBoundNode()` to find the function calls within the bound 
`functionDecl()` node.

However, if that's not workable, I think you can get rid of the 
`CalledFunctionsCollector` entirely and just use matchers directly within the 
`check()` function because by that point, you'll know exactly which AST nodes 
you want to traverse through.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:112-113
+  diag(RegisterCall->getBeginLoc(),
+   "exit-handler potentially calls an exit function. Handlers should "
+   "terminate by returning");
+  diag(HandlerDecl->getBeginLoc(), "handler function declared here",

whisperity wrote:
> Same as below, suggestion: "exit hander potentially calls exit function 
> instead of terminating normally with a return".
> 
> ("exit handler" and "exit function" without - is more in line with the SEI 
> CERT rule's phrasing too, they don't say "exit-handler".)
Slight tweak to the suggestion: `exit hander potentially calls an exit function 
instead of terminating normally with a return`



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:124-125
+  diag(RegisterCall->getSourceRange().getBegin(),
+   "exit-handler potentially calls a jump function. Handlers should "
+   "terminate by returning");
+  diag(HandlerDecl->getBeginLoc(), "handler function declared here",

whisperity wrote:
> This semi-sentence structure of starting with lowercase but also terminating 
> the sentence and leaving in another but unterminated sentences looks really 
> odd.
> 
> Suggestion: "exit handler potentially jumps instead of terminating normally 
> with a return"
Slight tweak here as well. I don't think we'll ever see a jump function other 
than longjmp for this rule, so what about writing `potentially calls 'longjmp'` 
instead of `potentially jumps`?



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:128
+   DiagnosticIDs::Note);
+  diag(CurrentUsage->getBeginLoc(), "jump function called here",
+   DiagnosticIDs::Note);

If you make the suggested change above, I'd do a similar change here.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:1
+//===--- Env32CCheck.cpp - clang-tidy 
-===//
+//

steakhal wrote:
> Env32CCheck.cpp -> ExitHandlerCheck.cpp
This comment was marked done but appears to still be an issue.


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

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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:64
+/// argument.
+void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) {
+  const auto IsRegisterFunction =

What happens if this test is run on C++ code calling the same functions? I see 
the rule is only applicable to C, for some reason... Should it be disabled from 
registering if by accident the check is enabled on a C++ source file?



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:68-71
+  0, declRefExpr(hasDeclaration(functionDecl().bind("handler_decl")))
+ .bind("handler_expr"));
+  Finder->addMatcher(
+  callExpr(IsRegisterFunction, HasHandlerAsFirstArg).bind("register_call"),

It is customary in most Tidy checks that use multiple binds to have the bind 
names defined as a symbol, instead of using just two string literals as if the 
bind has to be renamed, it's easy to mess it up.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:112-113
+  diag(RegisterCall->getBeginLoc(),
+   "exit-handler potentially calls an exit function. Handlers should "
+   "terminate by returning");
+  diag(HandlerDecl->getBeginLoc(), "handler function declared here",

Same as below, suggestion: "exit hander potentially calls exit function instead 
of terminating normally with a return".

("exit handler" and "exit function" without - is more in line with the SEI CERT 
rule's phrasing too, they don't say "exit-handler".)



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:124-125
+  diag(RegisterCall->getSourceRange().getBegin(),
+   "exit-handler potentially calls a jump function. Handlers should "
+   "terminate by returning");
+  diag(HandlerDecl->getBeginLoc(), "handler function declared here",

This semi-sentence structure of starting with lowercase but also terminating 
the sentence and leaving in another but unterminated sentences looks really odd.

Suggestion: "exit handler potentially jumps instead of terminating normally 
with a return"



Comment at: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c:31
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];

Which is the standard version this test file is set to analyse with? I don't 
see any `-std=` flag in the `RUN:` line.


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 282535.
gamesh411 added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,324 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-22]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-22]]:3: note: exit function called here
+  (void)at_quick_exit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-26]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-26]]:3: note: exit function called here
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-29]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-33]]:3: note: exit function called here
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-33]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-37]]:3: note: exit function called here
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-34]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-44]]:3: note: exit function called here
+  

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 added a comment.

I have updated the revision to use `llvm::move` algorithm (available thanks to 
@njames93).
Friendly ping :)


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 282533.
gamesh411 added a comment.

ensure lint in path


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,324 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-22]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-22]]:3: note: exit function called here
+  (void)at_quick_exit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-26]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-26]]:3: note: exit function called here
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-29]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-33]]:3: note: exit function called here
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-33]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-37]]:3: note: exit function called here
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-34]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-44]]:3: note: exit function called here
+  

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 282532.
gamesh411 added a comment.

use llvm::move algorithm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,324 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-22]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-22]]:3: note: exit function called here
+  (void)at_quick_exit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-26]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-26]]:3: note: exit function called here
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-29]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-33]]:3: note: exit function called here
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-33]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-37]]:3: note: exit function called here
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-34]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-44]]:3: note: exit function called here
+  

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 282529.
gamesh411 added a comment.

use llvm::move algorithm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp


Index: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
===
--- clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
+++ clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
@@ -12,7 +12,6 @@
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/SmallVector.h"
-#include 
 #include 
 #include 
 
@@ -141,8 +140,7 @@
 // Collect all the referenced FunctionDecls.
 Collector.TraverseStmt(CurrentDefWithBody->getBody());
 // Move the called functions to the worklist.
-std::move(Collector.begin(), Collector.end(),
-  std::back_inserter(CalledFunctions));
+llvm::move(Collector, std::back_inserter(CalledFunctions));
   }
 }
 


Index: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
===
--- clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
+++ clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
@@ -12,7 +12,6 @@
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/SmallVector.h"
-#include 
 #include 
 #include 
 
@@ -141,8 +140,7 @@
 // Collect all the referenced FunctionDecls.
 Collector.TraverseStmt(CurrentDefWithBody->getBody());
 // Move the called functions to the worklist.
-std::move(Collector.begin(), Collector.end(),
-  std::back_inserter(CalledFunctions));
+llvm::move(Collector, std::back_inserter(CalledFunctions));
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D83717#2155103 , @gamesh411 wrote:

> My thoughts exactly! I also thought about anchor-points as a feature in 
> file-check, as that would immensely increase the readability of the test-code 
> in such cases.


I've put in an RFC for that functionality, Think I CC'd you into it, if you 
have any comments about it I'd appreciate them.


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 added a comment.

In D83717#2155066 , @njames93 wrote:

> In D83717#2152762 , @gamesh411 wrote:
>
> > In D83717#2150977 , @njames93 
> > wrote:
> >
> > > Alternatively you could do something like this, though it would be a pain 
> > > https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment-gmock.cpp#L86
> >
> >
> > I have bitten the bullet, and have gone down this route. With relative 
> > numbering, the sections themselves are at least translation-invariant. Not 
> > the prettiest sight, tho.
> >  Thanks!
>
>
> I almost think it would be nice FileCheck supported some directive like:
>
>   // LINE-NAME: 
>
>
> And corresponding check lines:
>
>   [[]]
>   [[+N]] 
>   [[-N]]
>
>
> Would result in the ability to write checks like this:
>
>   void longjmp_handler() {// LINE-NAME: LONGJMP
>   longjmp(env, 255); 
>   }
>  
>   ...
>  
>   void foo(){
> atexit(longjmp_handler);
>  // CHECK-NOTES: :[[@LINE-1]]:3: warning: exit-handler potentially calls 
> a jump function. Handlers should terminate by returning [cert-env32-c]
> // CHECK-NOTES: :[[LONGJMP]]:1: note: handler function declared here
> // CHECK-NOTES: :[[LONGJMP+1]]:3: note: jump function called here
>   }
>
>
> Anyway that's a story for another day.


My thoughts exactly! I also thought about anchor-points as a feature in 
file-check, as that would immensely increase the readability of the test-code 
in such cases.


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

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D83717#2152762 , @gamesh411 wrote:

> In D83717#2150977 , @njames93 wrote:
>
> > Alternatively you could do something like this, though it would be a pain 
> > https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment-gmock.cpp#L86
>
>
> I have bitten the bullet, and have gone down this route. With relative 
> numbering, the sections themselves are at least translation-invariant. Not 
> the prettiest sight, tho.
>  Thanks!


I almost think it would be nice FileCheck supported some directive like:

  // LINE-NAME: 

And corresponding check lines:

  [[]]
  [[+N]] 
  [[-N]]

Would result in the ability to write checks like this:

  void longjmp_handler() {// LINE-NAME: LONGJMP
  longjmp(env, 255); 
  }
  
  ...
  
  void foo(){
atexit(longjmp_handler);
 // CHECK-NOTES: :[[@LINE-1]]:3: warning: exit-handler potentially calls a 
jump function. Handlers should terminate by returning [cert-env32-c]
// CHECK-NOTES: :[[LONGJMP]]:1: note: handler function declared here
// CHECK-NOTES: :[[LONGJMP+1]]:3: note: jump function called here
  }

Anyway that's a story for another day.


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 marked 2 inline comments as done.
gamesh411 added a comment.

In D83717#2153246 , @Eugene.Zelenko 
wrote:

> In D83717#2153245 , @gamesh411 wrote:
>
> > @Eugene.Zelenko I have just rebase-d, and seen that the release notes page 
> > itself was bumped to clang-tidy 12. I have added my check as a new check 
> > there. Should I also add the other subsections (like improvements in 
> > existing checks, and new check aliases), or authors will add them as needed?
>
>
> No, that sections will be filled when related changes will be made. Release 
> Notes are reset to blank state in master after branching release.


Ok, and thank you for your patience with this review! It was fruitful after 
all, and I have learned a thing or two about the project.


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

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D83717#2153245 , @gamesh411 wrote:

> @Eugene.Zelenko I have just rebase-d, and seen that the release notes page 
> itself was bumped to clang-tidy 12. I have added my check as a new check 
> there. Should I also add the other subsections (like improvements in existing 
> checks, and new check aliases), or authors will add them as needed?


No, that sections will be filled when related changes will be made. Release 
Notes are reset to blank state in master after branching release.


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 added a comment.

@Eugene.Zelenko I have just rebase-d, and seen that the release notes page 
itself was bumped to clang-tidy 12. I have added my check as a new check there. 
Should I also add the other subsections (like improvements in existing checks, 
and new check aliases), or authors will add them as needed?


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 278184.
gamesh411 added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,324 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-22]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-22]]:3: note: exit function called here
+  (void)at_quick_exit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-26]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-26]]:3: note: exit function called here
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-29]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-33]]:3: note: exit function called here
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-33]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-37]]:3: note: exit function called here
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-34]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-44]]:3: note: exit function called here
+  

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 278181.
gamesh411 added a comment.

tidy up release notes, make all new check entries uniform


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,324 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-22]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-22]]:3: note: exit function called here
+  (void)at_quick_exit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-26]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-26]]:3: note: exit function called here
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-29]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-33]]:3: note: exit function called here
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-33]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-37]]:3: note: exit function called here
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-34]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-44]]:3: note: exit function called 

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:87
+  ` check.
+  Finds functions registered by ``atexit`` and ``at_quick_exit`` that are 
calling
+  exit functions ``_Exit``, ``exit``, ``quick_exit`` or ``longjmp``.

gamesh411 wrote:
> Eugene.Zelenko wrote:
> > Eugene.Zelenko wrote:
> > > Please separate with empty line.
> > This was not done yet.
> Do you mean to separate the header line `Finds functions registered...` from 
> the link above? I think I was following the entry after this one as a 
> guideline instead of the one before.
Yes, I meant that. Please also same problem in entry below.


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:87
+  ` check.
+  Finds functions registered by ``atexit`` and ``at_quick_exit`` that are 
calling
+  exit functions ``_Exit``, ``exit``, ``quick_exit`` or ``longjmp``.

Eugene.Zelenko wrote:
> Eugene.Zelenko wrote:
> > Please separate with empty line.
> This was not done yet.
Do you mean to separate the header line `Finds functions registered...` from 
the link above? I think I was following the entry after this one as a guideline 
instead of the one before.


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 278178.
gamesh411 marked an inline comment as done.
gamesh411 added a comment.

add missing newline in release notes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,324 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-22]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-22]]:3: note: exit function called here
+  (void)at_quick_exit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-26]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-26]]:3: note: exit function called here
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-29]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-33]]:3: note: exit function called here
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-33]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-37]]:3: note: exit function called here
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-34]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-44]]:3: note: 

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:87
+  ` check.
+  Finds functions registered by ``atexit`` and ``at_quick_exit`` that are 
calling
+  exit functions ``_Exit``, ``exit``, ``quick_exit`` or ``longjmp``.

Eugene.Zelenko wrote:
> Please separate with empty line.
This was not done yet.


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 278176.
gamesh411 marked an inline comment as done.
gamesh411 added a comment.

.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,324 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-22]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-22]]:3: note: exit function called here
+  (void)at_quick_exit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-26]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-26]]:3: note: exit function called here
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-29]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-33]]:3: note: exit function called here
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-33]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-37]]:3: note: exit function called here
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-34]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-44]]:3: note: exit function called here
+  

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 278174.
gamesh411 added a comment.

fix documentation header


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,324 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-22]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-22]]:3: note: exit function called here
+  (void)at_quick_exit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-26]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-26]]:3: note: exit function called here
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-29]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-33]]:3: note: exit function called here
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-33]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-37]]:3: note: exit function called here
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-34]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-44]]:3: note: exit function called here
+  

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 marked 2 inline comments as done.
gamesh411 added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst:6
+
+This check implements SEI CERT rule ENV32-C by finding functions registered by
+``atexit`` and ``at_quick_exit`` that are calling exit functions ``_Exit``,

Eugene.Zelenko wrote:
> Please synchronize with statement in Release Notes. Please omit //This 
> check//.
Thanks! Fixed :)


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

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst:6
+
+This check implements SEI CERT rule ENV32-C by finding functions registered by
+``atexit`` and ``at_quick_exit`` that are calling exit functions ``_Exit``,

Please synchronize with statement in Release Notes. Please omit //This check//.


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 added a comment.

@steakhal @Eugene.Zelenko Thanks for checking this patch! I have tried my best 
to adhere to your suggestions.


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 marked an inline comment as done.
gamesh411 added a comment.

In D83717#2150977 , @njames93 wrote:

> Alternatively you could do something like this, though it would be a pain 
> https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment-gmock.cpp#L86


I have bitten the bullet, and have gone down this route. With relative 
numbering, the sections themselves are at least translation-invariant. Not the 
prettiest sight, tho.
Thanks!


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 marked 12 inline comments as done.
gamesh411 added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:132
+   "terminate by returning");
+  break;
+}

steakhal wrote:
> Why don't we `return` here?
> Same for the next `break`.
I use `break`, because it is more local to the concept of finishing the loop 
(ie the visitation algorithm), and that is what I want the reader of the code 
to receive after reading the line. In this particular case, `return` is 
equivalent to break but I would still keep it this way, because that is what I 
want to say, and if someone were to extend the code later, that early `return` 
inside the loop could surprise them.


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 278116.
gamesh411 marked 2 inline comments as done.
gamesh411 added a comment.

use move instead of copy
fix documentation issues
fix tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,324 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-22]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-22]]:3: note: exit function called here
+  (void)at_quick_exit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-26]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-26]]:3: note: exit function called here
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-29]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-33]]:3: note: exit function called here
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-33]]:1: note: handler function declared here
+  // CHECK-NOTES: :[[@LINE-37]]:3: note: exit function called here
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  // CHECK-NOTES: :[[@LINE-34]]:1: note: handler function declared here
+  // CHECK-NOTES: 

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Alternatively you could do something like this, though it would be a pain 
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment-gmock.cpp#L86


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

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D83717#2150099 , @gamesh411 wrote:

> extend with notes
>  apply minor fixes
>  tests are WIP until I figure out how to properly use file-check


If you add tests for notes, you will need to use `CHECK-MESSAGES-DAG` and 
`CHECK-NOTES-DAG` as notes wont appear in the same order as they are in the 
file.


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

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added reviewers: Szelethus, martong, steakhal, dkrupp.
Szelethus added a comment.

@Eugene.Zelenko Thanks for cleaning up revisions -- same as D69560#1725399 
, we are working in the same office 
and have worked on some forms of static analysis for a while now. Adding us as 
reviewers helps this revision being more visible. Its clear that we don't have 
the authority to approve changes to clang-tidy, but we can confidently request 
changes or add to the discussion while still respecting that.

I'll re-add ourselves to keep reviewing well oiled.


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

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:87
+  ` check.
+  Finds functions registered by ``atexit`` and ``at_quick_exit`` that are 
calling
+  exit functions ``_Exit``, ``exit``, ``quick_exit`` or ``longjmp``.

Please separate with empty line.



Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst:6
+
+This check implements SEI CERT rule ENV32-C.
+Description source: 
``_

Please synchronize with statement in Release Notes.



Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst:7
+This check implements SEI CERT rule ENV32-C.
+Description source: 
``_
+

Reference to original description usually placed at the end. 



Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst:12
+
+The C Standard provides three functions that cause an application to terminate 
normally: _Exit(), exit(), and quick_exit(). These are collectively called exit 
functions. When the exit() function is called, or control transfers out of the 
main() entry point function, functions registered with atexit() are called (but 
not at_quick_exit()). When the quick_exit() function is called, functions 
registered with at_quick_exit() (but not atexit()) are called. These functions 
are collectively called exit handlers.  When the _Exit() function is called, no 
exit handlers or signal handlers are called.
+

Please enclose all function names in double back-ticks. Same below.

Please don't use lines longer then 80 symbols.



Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst:16
+
+A nested call to an exit function is undefined behavior. (See undefined 
behavior 182.) This behavior can occur only when an exit function is invoked 
from an exit handler or when an exit function is called from within a signal 
handler. (See SIG30-C. Call only asynchronous-safe functions within signal 
handlers.)
+

Is //SIG30-C// implemented in Clang-tidy? If not, reference should be removed.


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

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Unfortunately, I'm not qualified enough to have much to say.




Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:1
+//===--- Env32CCheck.cpp - clang-tidy 
-===//
+//

Env32CCheck.cpp -> ExitHandlerCheck.cpp



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:38
+bool isExitFunction(StringRef FnName) {
+  return FnName == EF__Exit || FnName == EF_exit || FnName == EF_quick_exit;
+}

If `EF__Exit`, `EF_exit` and `EF_quick_exit` referred only once, we could 
simply inline those,

Same for the `isJumpFunction`.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:60
+  /// Iteration over the collector is iteration over the found FunctionDecls.
+  auto begin() const -> decltype(CalledFunctions.begin()) {
+return CalledFunctions.begin();

IMO `-> decltype(CalledFunctions.begin())` is unnecessary.
Return type deduction does the right thing for this case.

Same for the `auto end() ...`



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:84
+  hasArgument(
+0, // the first argument is the handler function
+declRefExpr(

If you disabled clang-format for having inline comments, Then you could create 
smaller matchers and give names for them.

Something similar to this:
```lang=cpp
const auto DesciptiveName = hasArgument(0, 
declRefExpr(hasDeclaration(functionDecl().bind("handler_decl";
```



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:132
+   "terminate by returning");
+  break;
+}

Why don't we `return` here?
Same for the next `break`.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:148-154
+// Collect all the referenced FunctionDecls.
+Collector.TraverseStmt(CurrentDefWithBody->getBody());
+// Add called functions to the worklist.
+std::copy(Collector.begin(), Collector.end(),
+  std::back_inserter(CalledFunctions));
+// Reset the ASTVisitor instance results.
+Collector.clear();

nit: I would clear the `Collector` before the `TraverseStmt`.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h:20
+/// Checker for SEI CERT rule ENV32-C
+/// All exit handlers must return normal.
+/// Exit handlers must terminate by returning. It is important and potentially

In the documentation you use the:
clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
> All exit handlers must return normally.

You should be consistent.


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 marked 10 inline comments as done.
gamesh411 added a comment.

Thanks @njames93 :) I have extended the check with notes, but I have to figure 
out how to appease file-check, so its still WIP until I do.




Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:72
+void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) {
+  // clang-format off
+  Finder->addMatcher(

njames93 wrote:
> Is there a reason  for disabling format here? How messy is the matcher code 
> when formatted?
It is not that bad after all. Especially with the `hasAnyName` predicate.



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

njames93 wrote:
> nit:
> ```
> if (!SeenFunctions.insert(Current).second)
>   continue;
> ```
Neat!


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

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 277796.
gamesh411 added a comment.

extend with notes
apply minor fixes
tests are WIP until I figure out how to properly use file-check


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,324 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+// CHECK-NOTES: :[[@LINE+2]]:1: note: handler function declared here
+// CHECK-NOTES: :[[@LINE+1]]:1: note: handler function declared here
+void call__Exit() {
+  _Exit(0);
+  // CHECK-NOTES: :[[@LINE-1]]:3: note: exit function called here
+  // CHECK-NOTES: :[[@LINE-2]]:3: note: exit function called here
+  // CHECK-NOTES: :[[@LINE-3]]:3: note: exit function called here
+  // CHECK-NOTES: :[[@LINE-4]]:3: note: exit function called here
+  // CHECK-NOTES: :[[@LINE-5]]:3: note: exit function called here
+  // CHECK-NOTES: :[[@LINE-6]]:3: note: exit function called here
+  // CHECK-NOTES: :[[@LINE-7]]:3: note: exit function called here
+  // CHECK-NOTES: :[[@LINE-8]]:3: note: exit function called here
+}
+
+// CHECK-NOTES: :[[@LINE+2]]:1: note: handler function declared here
+// CHECK-NOTES: :[[@LINE+1]]:1: note: handler function declared here
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+// CHECK-NOTES: :[[@LINE+2]]:1: note: handler function declared here
+// CHECK-NOTES: :[[@LINE+1]]:1: note: handler function declared here
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+// CHECK-NOTES: :[[@LINE+2]]:1: note: handler function declared here
+// CHECK-NOTES: :[[@LINE+1]]:1: note: handler function declared here
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning 

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

[Nathan James via Phabricator via cfe-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
+  ` 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 277590.
gamesh411 added a comment.

mention iterator header


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,256 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test_conditional__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call_call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+// Non-compliant solutions calling exit
+
+void 

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 277588.
gamesh411 added a comment.

use std::copy algorithm
polish


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,256 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test_conditional__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call_call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+// Non-compliant solutions calling exit
+

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 277581.
gamesh411 added a comment.

fix test diag location changes resulting from autoformatting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,256 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test_conditional__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call_call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+// Non-compliant 

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 277575.
gamesh411 added a comment.

simplify VisitCallExpr


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,256 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test_conditional__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call_call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+// Non-compliant solutions calling exit
+

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 updated this revision to Diff 277571.
gamesh411 added a comment.

update includes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,256 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test_conditional__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call_call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+// Non-compliant solutions calling exit
+
+void 

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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 created this revision.
Herald added subscribers: llvm-commits, cfe-commits, martong, steakhal, 
Szelethus, dkrupp, xazax.hun, whisperity, mgorny.
Herald added projects: clang, LLVM.

Add check for the SEI CERT item ENV32-C which dictates that exit handler
functions should terminate by returning as opposed to calling exit-functions
or jumping out of the handler function body with 'longjmp'. The handler
registration functions 'atexit and 'at_quick_exit',  exit functions '_Exit',
'exit', 'quick_exit are checked.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D83717

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/cert/BUILD.gn
@@ -19,6 +19,7 @@
 "CommandProcessorCheck.cpp",
 "DefaultOperatorNewAlignmentCheck.cpp",
 "DontModifyStdNamespaceCheck.cpp",
+"ExitHandlerCheck.cpp",
 "FloatLoopCounter.cpp",
 "LimitedRandomnessCheck.cpp",
 "MutatingCopyCheck.cpp",
Index: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
@@ -0,0 +1,256 @@
+// RUN: %check_clang_tidy %s cert-env32-c %t
+
+// --
+// EXIT FUNCTIONS
+// --
+
+// No handlers are invoked when _Exit is called.
+void _Exit(int __status);
+
+// Handlers registered by atexit are invoked in reverse order when exit is
+// called.
+void exit(int __status);
+
+// Handlers registered by at_quick_exit are invoked in reverse order when
+// quick_exit is called.
+void quick_exit(int __status);
+
+// 
+// HANDLER REGISTRATION
+// 
+
+// Register handlers to run when exit is called.
+int atexit(void (*__func)(void));
+
+// Register handlers to run when exit is called.
+int at_quick_exit(void (*__func)(void));
+
+// --
+// Setjmp/longjmp
+// --
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
+int setjmp(jmp_buf);
+void longjmp(jmp_buf, int);
+
+// Compliant solutions
+
+void cleanup1() {
+  // do cleanup
+}
+
+void cleanup2() {
+  // do cleanup
+}
+
+void test_atexit_single_compliant() {
+  (void)atexit(cleanup1);
+}
+
+void test_atexit_multiple_compliant() {
+  (void)atexit(cleanup1);
+  (void)atexit(cleanup2);
+}
+
+void test_at_quick_exit_single_compliant() {
+  (void)at_quick_exit(cleanup1);
+}
+
+void test_at_quick_exit_multiple_compliant() {
+  (void)at_quick_exit(cleanup1);
+  (void)at_quick_exit(cleanup2);
+}
+
+// Non-compliant solutions calling _Exit
+
+void call__Exit() {
+  _Exit(0);
+}
+
+void call_call__Exit() {
+  call__Exit();
+}
+
+extern int unknown__Exit_flag;
+
+void call__Exit_conditionally() {
+  if (unknown__Exit_flag)
+call__Exit();
+}
+
+void call_call__Exit_conditionally() {
+  call__Exit_conditionally();
+}
+
+void test__Exit_called_directly() {
+  (void)atexit(call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call_call__Exit);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test_conditional__Exit_called_directly() {
+  (void)atexit(call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+  (void)at_quick_exit(call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: exit-handler potentially calls an exit function. Handlers should terminate by returning [cert-env32-c]
+};
+
+void test_conditional__Exit_called_indirectly() {
+  (void)atexit(call_call__Exit_conditionally);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: