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

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.

================
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+  Finder->addMatcher(
+      declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+                                              parameterCountIs(0))))
+          .bind("randomGenerator"),
----------------
aaron.ballman wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > Prazek wrote:
> > > > aaron.ballman wrote:
> > > > > Prazek wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > This should be looking at a callExpr() rather than a declRefExpr(), should it not?
> > > > > > I was also thinking about this, but this is actually better, because it will also match with binding rand with function pointer.
> > > > > 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.)
> > > > 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.
> > > > 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).
> > In case you let binding to function pointer you introduce potential false negatives which is worse in this case in my opinion. 
> 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.
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D22346