Page MenuHomePhabricator

[Clang-tidy] CERT-MSC50-CPP (std:rand() )
ClosedPublic

Authored by falho on Jul 14 2016, 5:13 AM.

Details

Diff Detail

Event Timeline

falho updated this revision to Diff 63955.Jul 14 2016, 5:13 AM
falho retitled this revision from to CERT-MSC50-CPP (std:rand() ).
falho updated this object.
falho updated this revision to Diff 67728.Aug 11 2016, 12:41 PM
falho retitled this revision from CERT-MSC50-CPP (std:rand() ) to [Clang-tidy] CERT-MSC50-CPP (std:rand() ).Aug 11 2016, 2:21 PM
falho set the repository for this revision to rL LLVM.
falho edited subscribers, added: Eugene.Zelenko, cfe-commits; removed: xazax.hun, o.gyorgy, alex, nemanjai.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

docs/clang-tidy/checks/cert-msc50-cpp.rst
5

Should be same length as section name above.

7

Please use check and highlight std::rand() with ``.

Prazek accepted this revision.Aug 14 2016, 1:25 AM
Prazek edited edge metadata.

LGTM with the fixes of docs.

clang-tidy/cert/LimitedRandomnessCheck.cpp
32

I am not sure about the diagnostics convention, it is possible that you should replace comma with semicolon.

This revision is now accepted and ready to land.Aug 14 2016, 1:25 AM
aaron.ballman requested changes to this revision.Aug 14 2016, 4:20 AM
aaron.ballman edited edge metadata.

Thank you for working on this check!

clang-tidy/cert/CERTTidyModule.cpp
37

Please place this under a MSC heading rather than DCL.

This check should additionally be listed as cert-msc30-c (https://www.securecoding.cert.org/confluence/display/c/MSC30-C.+Do+not+use+the+rand%28%29+function+for+generating+pseudorandom+numbers).

clang-tidy/cert/LimitedRandomnessCheck.cpp
23–24

This should be looking at a callExpr() rather than a declRefExpr(), should it not?

32

This diagnostic is incorrect for C code. Should only suggest C++ <random> in C++ mode. Also, it should be a semicolon rather than a comma in the diagnostic text.

clang-tidy/cert/LimitedRandomnessCheck.h
20

Both sentences are correct, but it doesn't clarify why std::rand() is bad. Should add a sentence between these two that says something about why std::rand() in particular is called out rather than the other pseudorandom number generators.

docs/clang-tidy/checks/cert-msc50-cpp.rst
7

The same argument could be used to disallow C++ <random> generators that aren't true truly random sources. Should specify more about why rand() is particularly bad.

docs/clang-tidy/checks/list.rst
21

Please also add a cert-msc30-c file with a redirect (like fio38-c from above).

test/clang-tidy/cert-limited-randomness.cpp
2

Please also add a test for C since the check works there as well, and will have different diagnostic text.

This revision now requires changes to proceed.Aug 14 2016, 4:20 AM
alex resigned from this revision.Aug 14 2016, 6:20 AM
alex removed a reviewer: alex.
Prazek added inline comments.Aug 14 2016, 11:59 AM
clang-tidy/cert/LimitedRandomnessCheck.cpp
23–24

I was also thinking about this, but this is actually better, because it will also match with binding rand with function pointer.

aaron.ballman added inline comments.Aug 14 2016, 1:37 PM
clang-tidy/cert/LimitedRandomnessCheck.cpp
23–24

True, but a DeclRefExpr doesn't mean it's a function call. Binding the function is not contrary to the CERT rule, just calling it. For instance, the following pathological case will be caught by this check:

if (std::rand) {}

The behavior of this check should be consistent with cert-env33-c, which only looks at calls. (If we really care about bound functions, we'd need flow control analysis, and I think that's overkill for both of those checks, but wouldn't be opposed to someone writing the flow analysis if they really wanted to.)

falho added a comment.Aug 14 2016, 2:02 PM

Hi! Thanks for the reviews! I will be off for a few days so I will start working on it when Im back. Greetz! Benedek

Prazek added inline comments.Aug 14 2016, 2:08 PM
clang-tidy/cert/LimitedRandomnessCheck.cpp
23–24

It would indeed fire on this pathological case, but I don't think we should care about cases like this, because no one is writing code like this (and if he would then it would probably be a bug).
I don't think that there is much code that binds pointer to std::rand either, but I think it would be good to display warning for this, because even if the function would be never called, then it means that this is a bug, and if it would be called then it would be nice to tell user that rand might be used here.

Anyway I don't oppose for changing it to callExpr, but I think it is better this way.

aaron.ballman added inline comments.Aug 14 2016, 3:10 PM
clang-tidy/cert/LimitedRandomnessCheck.cpp
23–24

It would indeed fire on this pathological case, but I don't think we should care about cases like this, because no one is writing code like this (and if he would then it would probably be a bug).

It would be a known false-positive for a check designed to conform to a particular coding standard. When deviations have come up in the past for various coding standards, we've added an option to enable the additional functionality, which I don't think would be reasonable in this case. Alternatively, the CERT guideline could be modified, but that is unlikely to occur because binding the function pointer is not a security concern (only calling the function).

xazax.hun added inline comments.Aug 15 2016, 12:07 AM
clang-tidy/cert/LimitedRandomnessCheck.cpp
23–24

In case you let binding to function pointer you introduce potential false negatives which is worse in this case in my opinion.

aaron.ballman added inline comments.Aug 15 2016, 5:37 AM
clang-tidy/cert/LimitedRandomnessCheck.cpp
23–24

Basically: this half-measure is sufficient for the CERT coding rule, but isn't ideal. The ideal check isn't likely to uncover many more cases than the half-measure, which is why it was not implemented in the past. If someone wants to implement the whole-measure, that's great! But implementing a half, half-measure that isn't consistent with other, similar checks is the wrong thing to do.

xazax.hun added inline comments.Aug 15 2016, 5:46 AM
clang-tidy/cert/LimitedRandomnessCheck.cpp
23–24

You can not implement an ideal checker. In general, it is undecidable whether std::rand is called or not. (You can easily create an example where you would need to solve the halting problem in order to decide whether std::rand is called.)

Since the ideal checker is infeasible the question is whether you are OK with false positives or false negatives. The approach you are suggesting result in false negatives. The current approach results in false positives. I think, for this security checker, a false positive is much less serious to have than a false negative. Moreover, I doubt that people write code where such false positives are intended and the code should not be changed. But in case you can think of an example, please let us know.

xazax.hun added inline comments.Aug 15 2016, 5:52 AM
clang-tidy/cert/LimitedRandomnessCheck.cpp
23–24

I think consisteny with other checks are not always a good argument. You might want to ask what is the expected false positive and false nagtive rate from a check, and what is the guarantee that a user expects from a check. And I think base on that it is a unique decision that should be considered independently for each check. In this case I think it is more valuable to have a guarantee that in case all of the code is covered, std::rand() is not invoked. Using a callExpr instead of declRefExpr we would loose this guarantee at the cost of not reporting some false positive cases that are unlikely to annoy anyone anyways.

aaron.ballman added inline comments.Aug 15 2016, 5:52 AM
clang-tidy/cert/LimitedRandomnessCheck.cpp
23–24

You can not implement an ideal checker. In general, it is undecidable whether std::rand is called or not. (You can easily create an example where you would need to solve the halting problem in order to decide whether std::rand is called.)

I said "ideal", not "perfect", but we're splitting hairs. You are correct, you're never going to get perfect clarity without runtime instrumentation. By "ideal", I meant "something that actually pays attention to the bound value from the point it is bound to the point the function pointer is called." Simply having the address of the function is not a security concern; calling it is.

I think, for this security checker, a false positive is much less serious to have than a false negative.

We'll have to agree to disagree. As the person who maintains the CERT rules (and their checkers), my preference for right now is to use callExpr().

falho marked 5 inline comments as done.Sep 13 2016, 9:23 AM
falho added inline comments.
clang-tidy/cert/LimitedRandomnessCheck.cpp
23–24

Thank you for the reviews! So what is the conclusion? Should I change to callExpr()?

dkrupp added a subscriber: dkrupp.Sep 13 2016, 9:41 AM
aaron.ballman added inline comments.Sep 14 2016, 10:58 AM
clang-tidy/cert/LimitedRandomnessCheck.cpp
23–24

My preference is that the check use callExpr() -- otherwise the check will trigger as a false positive for code that is allowed by MSC50-CPP and MSC30-C. If others feel strongly that declRefExpr() is the better approach, then I'd like CommandProcessorCheck::registerMatchers(), SetLongJmpCheck::registerMatchers(), and StrToNumCheck::registerMatchers() changed to behave similarly (though not as part of this patch).

falho marked 5 inline comments as done.Oct 17 2016, 2:30 PM
falho updated this revision to Diff 74908.Oct 17 2016, 2:50 PM
falho edited edge metadata.
falho removed rL LLVM as the repository for this revision.

updated diff according to first reviews

Thank you for continuing your efforts on this, I have just a few minor nits remaining.

clang-tidy/cert/LimitedRandomnessCheck.cpp
36

For C code, this diagnostic will read strangely due to the trailing semicolon. You should move the semicolon into the msg above. Perhaps we can also drop "function" from the diagnostic as well.

docs/clang-tidy/checks/cert-msc50-cpp.rst
4

This should be cert-msc50-cpp instead.

docs/clang-tidy/checks/list.rst
21

This should be cert-msc30-c

falho marked 3 inline comments as done.Oct 19 2016, 2:36 AM

I noticed you marked several comments as done, but the patch is not updated with changes. Have I missed something?

falho updated this revision to Diff 75460.Oct 21 2016, 12:14 PM
falho edited edge metadata.

changes according to 2nd review

clang-tidy/cert/.LimitedRandomnessCheck.cpp.swo was added and should not have been; also, there's one minor issue with the diagnostic wording that is still outstanding.

clang-tidy/cert/LimitedRandomnessCheck.cpp
36

This comment does not appear to have been handled -- the semicolon is still present even when msg is empty.

falho updated this revision to Diff 75935.Oct 26 2016, 1:34 PM
falho marked an inline comment as done.

removed semicolon, and replaced it with a comma that only appears in .cpp diagnostics
test cases corrected according to this

removed junk .swo file

removed semicolon, and replaced it with a comma that only appears in .cpp diagnostics

The semicolon was the correct punctuator to use, but thank you for moving it into the cpp message.

test cases corrected according to this

removed junk .swo file

Thanks!

clang-tidy/cert/LimitedRandomnessCheck.cpp
31

Switch the "," to ";", and remove the curly braces.

falho updated this revision to Diff 76072.Oct 27 2016, 11:42 AM
falho marked an inline comment as done.

in cpp diagnostics message: comma changed back to semicolon, + curly braces removed
testfiles corrected accordingly

aaron.ballman accepted this revision.Oct 27 2016, 1:27 PM
aaron.ballman edited edge metadata.

LGTM, thank you for working on this!

This revision is now accepted and ready to land.Oct 27 2016, 1:27 PM
falho added a comment.Oct 27 2016, 1:32 PM

Cool! Thank you for the reviews!

Cool! Thank you for the reviews!

If you don't have commit privileges, let me know and I'm happy to commit on your behalf.

falho added a comment.Nov 1 2016, 4:16 AM

Thanks but I think I will try it!

falho added a comment.Nov 2 2016, 1:34 AM

I just figured out that I don't have right to commit to llvm so I would appreciate if you could commit this check for me. Do you need any info about me? Thank you!

aaron.ballman closed this revision.Nov 2 2016, 7:26 AM

Thanks! I've commit in r285809