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"),
----------------
xazax.hun wrote:
> 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.
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.
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
