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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
