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: > 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.
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