Page MenuHomePhabricator

[clang-tidy] Add check fo SEI CERT item ENV32-C
Needs ReviewPublic

Authored by gamesh411 on Jul 13 2020, 2:11 PM.

Details

Summary

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, and quick_exit are checked.

Diff Detail

Unit TestsFailed

TimeTest
110 mswindows > LLVM.Other::change-printer.ll
Script: -- : 'RUN: at line 6'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\opt.exe -S -print-changed -passes=instsimplify 2>&1 -o /dev/null < C:\ws\w16-1\llvm-project\premerge-checks\llvm\test\Other\change-printer.ll | c:\ws\w16-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16-1\llvm-project\premerge-checks\llvm\test\Other\change-printer.ll --check-prefix=CHECK_SIMPLE

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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!

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

Eugene.Zelenko added inline comments.Jul 15 2020, 7:00 AM
clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
7

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

gamesh411 updated this revision to Diff 278174.Jul 15 2020, 7:06 AM

fix documentation header

gamesh411 marked 2 inline comments as done.Jul 15 2020, 7:06 AM
gamesh411 added inline comments.
clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
7

Thanks! Fixed :)

gamesh411 updated this revision to Diff 278176.Jul 15 2020, 7:07 AM
gamesh411 marked an inline comment as done.

.

Eugene.Zelenko added inline comments.Jul 15 2020, 7:11 AM
clang-tools-extra/docs/ReleaseNotes.rst
87

This was not done yet.

gamesh411 updated this revision to Diff 278178.Jul 15 2020, 7:15 AM
gamesh411 marked an inline comment as done.

add missing newline in release notes

gamesh411 added inline comments.Jul 15 2020, 7:16 AM
clang-tools-extra/docs/ReleaseNotes.rst
87

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.

Eugene.Zelenko added inline comments.Jul 15 2020, 7:18 AM
clang-tools-extra/docs/ReleaseNotes.rst
87

Yes, I meant that. Please also same problem in entry below.

gamesh411 updated this revision to Diff 278181.Jul 15 2020, 7:21 AM

tidy up release notes, make all new check entries uniform

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

@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.

gamesh411 marked 2 inline comments as done.Jul 15 2020, 7:41 AM

@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.

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: <LINENAME>

And corresponding check lines:

[[<LINENAME>]]
[[<LINENAME>+N]] 
[[<LINENAME>-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.

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: <LINENAME>

And corresponding check lines:

[[<LINENAME>]]
[[<LINENAME>+N]] 
[[<LINENAME>-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.

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.

gamesh411 updated this revision to Diff 282529.Aug 3 2020, 1:50 AM

use llvm::move algorithm

gamesh411 updated this revision to Diff 282532.Aug 3 2020, 1:58 AM

use llvm::move algorithm

gamesh411 updated this revision to Diff 282533.Aug 3 2020, 2:01 AM

ensure lint in path

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

whisperity added inline comments.Aug 3 2020, 3:54 AM
clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
65

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?

69–72

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.

113–114

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".)

125–126

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"

clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
32

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

aaron.ballman added inline comments.Aug 3 2020, 5:06 AM
clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
2

This comment was marked done but appears to still be an issue.

34

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

65

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

71

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.

113–114

Slight tweak to the suggestion: exit hander potentially calls an exit function instead of terminating normally with a return

125–126

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?

129

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

whisperity added inline comments.Aug 3 2020, 5:25 AM
clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
34

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.
65

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

125–126

😉 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.

njames93 added inline comments.Aug 3 2020, 7:06 AM
clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
32

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

34

That only works because the logic inside the hasNamematcher

37

Ditto

gamesh411 updated this revision to Diff 282837.Aug 4 2020, 2:13 AM
gamesh411 marked an inline comment as done.

rename file name in header

aaron.ballman added inline comments.Aug 5 2020, 4:43 AM
clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
34

That's the bit I was worried about, too, thanks for confirming @njames93.

65

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.

whisperity added inline comments.Aug 5 2020, 5:05 AM
clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
65

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.

aaron.ballman added inline comments.Aug 5 2020, 5:26 AM
clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
65

Thanks, I'll pass this suggestion along!

gamesh411 updated this revision to Diff 289597.Sep 2 2020, 3:46 PM

only consider global and ::std scope handlers

aaron.ballman added inline comments.Sep 3 2020, 6:26 AM
clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
38

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

gamesh411 marked 2 inline comments as done.

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

gamesh411 marked 13 inline comments as done.Sep 17 2020, 12:21 AM

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
clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
71

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.

clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c
32

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.

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

gamesh411 marked 6 inline comments as done.Sep 17 2020, 12:35 AM
gamesh411 marked 2 inline comments as done.Sep 17 2020, 12:46 AM
gamesh411 updated this revision to Diff 292488.Sep 17 2020, 7:09 AM

Update commit message

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?

clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp
102

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?

115

Similar suggestion here: exit handler potentially calls 'longjmp' which may fail to run other exit handlers?

clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
8

terminate?

10

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.