Page MenuHomePhabricator

[analyzer] CERT: POS34-C
ClosedPublic

Authored by zukatsinadze on Dec 12 2019, 1:21 PM.

Details

Summary

This patch introduces a new checker:
alpha.security.cert.pos.34c

This checker is implemented based on the following rule:
https://wiki.sei.cmu.edu/confluence/display/c/POS34-C.+Do+not+call+putenv%28%29+with+a+pointer+to+an+automatic+variable+as+the+argument
The check warns if putenv function is
called with automatic storage variable as an argument.

Diff Detail

Event Timeline

zukatsinadze created this revision.Dec 12 2019, 1:21 PM
zukatsinadze edited the summary of this revision. (Show Details)Dec 12 2019, 1:21 PM
Charusso added a subscriber: aaron.ballman.
Charusso added inline comments.
clang/docs/analyzer/checkers.rst
1954

I would remove that line.

1966

I think it is clear it is a CERT-checker.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
839

I would write `putenv` -> 'putenv' and the CERT rule-number should be clear from that point so you could emit it.

clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
53

I think you could shrink that into:

!IsPossiblyAutoVar && !isa<StackSapceRegion>(MSR)

What you have specified is a negation of

!isa<StackSapceRegion>(MSR) && !isa<CodeSpaceRegion>(MSR) && !isa<GlobalInternalSpaceRegion>(MSR)

~ according to: https://clang.llvm.org/doxygen/classclang_1_1ento_1_1MemSpaceRegion.html

where the CodeSpaceRegion and GlobalInternalSpaceRegion is not that interesting for the checker.

59

@NoQ, it is from your documentation. Would we prefer that or the registerChecker(CheckerManager &) is the new way to construct the BugType? I believe the latter is more appropriate.

61

We would like to point out the allocation's site, where it comes from.
We have two facilities to do so: MallocBugVisitor to track dynamic allocation and trackExpressionValue() to track any kind of an expression.

You could find examples from my CERT-checker: D70411. The rough idea is that:

// Track the argument.
if (isa<StackSpaceRegion>(MSR)) {
  bugreporter::trackExpressionValue(C.getPredecessor(), ArgExpr, *Report);
} else if (const SymbolRef Sym = ArgV.getAsSymbol()) { // It is a `HeapSpaceRegion`
  Report->addVisitor(allocation_state::getMallocBRVisitor(Sym));
}

~ here you need to copy-paste the getMallocBRVisitor() from my review. Sorry!

clang/test/Analysis/cert/pos34-c.cpp
5

Could you inject the rule's page please?

14

I would name the rule properly, as a sentence like: Example from the CERT rule's page.

My idea was that to have a /cert/pos34-c.cpp which only consists of the rule's page stuff mentioning the rule's page on the top. And having a separate file which only consists of arbitrary tests called like /cert/pos34-c-fp-suppression.cpp as it shows some false-positive suppression what you have found on tests or something. Like @aaron.ballman wrote two tests in D70823 and I would copy-paste them from his comment.

NoQ added a comment.Dec 13 2019, 2:05 PM

Thanks! This looks like a simple and efficient check. I have one overall suggestion.

Currently the check may warn on non-bugs of the following kind:

void foo() {
  char env[] = "NAME=value";
  putenv(env);
  doStuff();
  putenv("NAME=anothervalue");
}

I.e., it's obviously harmless if the local variable pointer is removed from the environment before it goes out of scope. Can we instead warn when the *last* putenv() on the execution path through the current stack frame is a local variable (that goes out of scope at the end of the stack frame)?

That'd allow the checker to be enabled by default, which will give a lot more users access to it. Otherwise we'll have to treat it as an opt-in check and users will only enable it when they know about it, which is much less usefulness.

clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
28

The modern idiom is BugType BT{this, "Bug kind", "Bug category"}; - and drop the lazy initialization as well.

36–38

The modern idiom here is CallDescription.

43–44

You can call getMemorySpace() directly on any region.

46–53

Simply check whether the memory space is a stack memory space. This should be a one-liner.

59

Replied above^^

61

I'm not sure it's valid to use C.getPredecessor() for tracking; it might get you into trouble for the same reason that using C.getPredecessor() as error node will get you into trouble. Please use the error node itself instead.

Addressed most of the inline comments.

zukatsinadze marked 10 inline comments as done.Dec 14 2019, 3:00 PM
zukatsinadze marked 4 inline comments as done.Dec 14 2019, 3:05 PM
zukatsinadze added inline comments.
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
839

Oops. Forgot this one. Will fix it later.

clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
46–53

I could not get rid of isPossiblyAutoVar and GlobalInternalSpaceRegion.

clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
16

I need isPossiblyAutoVar for this type.

21

And GlobalInternalSpaceRegion for this.

In D71433#1784238, @NoQ wrote:

Thanks! This looks like a simple and efficient check. I have one overall suggestion.

Currently the check may warn on non-bugs of the following kind:

void foo() {
  char env[] = "NAME=value";
  putenv(env);
  doStuff();
  putenv("NAME=anothervalue");
}

I.e., it's obviously harmless if the local variable pointer is removed from the environment before it goes out of scope. Can we instead warn when the *last* putenv() on the execution path through the current stack frame is a local variable (that goes out of scope at the end of the stack frame)?

That'd allow the checker to be enabled by default, which will give a lot more users access to it. Otherwise we'll have to treat it as an opt-in check and users will only enable it when they know about it, which is much less usefulness.

@NoQ I like the idea, but I am not really sure how to do that. I started working on Static Analyzer just lask week.

NoQ added a comment.Dec 14 2019, 6:42 PM

@NoQ I like the idea, but I am not really sure how to do that. I started working on Static Analyzer just lask week.

Let's get the initial attempt right first, and delay this for the next patch. You could accomplish this by keeping track of the last putenv() in a program state trait and moving the warning in checkEndFunction().

clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
16

This test is pretty questionable. There is no indication in the code that a points to an automatic variable.

21

This test is wrong. "hello" is not an automatic variable.

  • Removed extra test
  • Used CallDescription for checking call.
zukatsinadze marked 6 inline comments as done.Dec 15 2019, 1:22 AM
Charusso accepted this revision.Jan 7 2020, 10:18 AM

I have created a notes.cpp test-file to test the notes in my checkers, but I think this checker is fine without that test file. @NoQ, what do you think?

This revision is now accepted and ready to land.Jan 7 2020, 10:18 AM
In D71433#1784238, @NoQ wrote:

Currently the check may warn on non-bugs of the following kind:

void foo() {
  char env[] = "NAME=value";
  putenv(env);
  doStuff();
  putenv("NAME=anothervalue");
}

That could be the next round as a follow-up patch as the next semester starts in February after the 10.0.0 release-tag has been set, so both of the patches could land in LLVM 11. We have not seen any putenv() in the wild yet.

NoQ added inline comments.Feb 3 2020, 7:58 AM
clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
58–59

This is impossible because StackSpaceRegion and HeapSpaceRegion do not overlap and above you checked that it's the former.

clang/test/Analysis/cert/pos34-c.cpp
7

Btw - CERT has minified links!

Addressed new inline comments.

zukatsinadze marked 2 inline comments as done.Feb 7 2020, 3:39 AM
In D71433#1784238, @NoQ wrote:

Currently the check may warn on non-bugs of the following kind:

void foo() {
  char env[] = "NAME=value";
  putenv(env);
  doStuff();
  putenv("NAME=anothervalue");
}

That could be the next round as a follow-up patch as the next semester starts in February [...]

Well, the next semester is about to start. Could you implement that request please?

In D71433#1784238, @NoQ wrote:

Currently the check may warn on non-bugs of the following kind:

void foo() {
  char env[] = "NAME=value";
  putenv(env);
  doStuff();
  putenv("NAME=anothervalue");
}

That could be the next round as a follow-up patch as the next semester starts in February [...]

Well, the next semester is about to start. Could you implement that request please?

Sure. I plan to start working on it next week.

Szelethus accepted this revision.Feb 7 2020, 5:24 AM

This is a very neat checker, the source code reads so easily, we might as well put it as the official CERT rule description.

I think adding the non-compliant and compliant code examples would be nice. I also wrote some inline comments, but I'm fine with leaving them for a later patch. LGTM, granted that @NoQ and @Charusso are happy.

clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
16–22

We could turns these into llvm::StringLiterals one day.

clang/lib/StaticAnalyzer/Checkers/AllocationState.h
28–30 ↗(On Diff #243127)

Is this deadcode now?

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
3383–3386 ↗(On Diff #243127)

And this?

clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
51

How about The 'putenv' function should not be called with arguments that have automatic storage. But I guess we could leave wordsmithing for later.

56

This should always be true, since we bail out a couple lines up if it isn't, right?

zukatsinadze marked an inline comment as done.
  • Removed dead code.
  • Removed unnecessary if condition.
  • Changed error phrasing.
zukatsinadze marked 2 inline comments as done.Feb 7 2020, 5:56 AM
zukatsinadze added inline comments.
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
3383–3386 ↗(On Diff #243127)

Forgot to delete. Thanks

I think for an alpha checker this is ready to land if you're ready -- do you have commit access or need assistance?

I think for an alpha checker this is ready to land if you're ready -- do you have commit access or need assistance?

Thank you. @Charusso will help.

This revision was automatically updated to reflect the committed changes.