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"),
----------------
falho wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > 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.
> > > > 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().
> > >
> > 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.
> Thank you for the reviews! So what is the conclusion? Should I change to
> callExpr()?
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).
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
