aaron.ballman 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"),
----------------
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.
Repository:
rL LLVM
https://reviews.llvm.org/D22346
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
