[cfe-dev] More static analysis...

[Ben Laurie]

On Mon, Mar 30, 2009 at 1:14 PM, Zhongxing Xu <xuzhongxing at gmail.com> wrote:
>
>
> 2009/3/30 Ben Laurie <benl at google.com>
>>
>> On Fri, Feb 27, 2009 at 6:02 AM, Ted Kremenek <kremenek at apple.com> wrote:
>> >
>> > On Feb 26, 2009, at 2:18 AM, Ben Laurie wrote:
>> >
>> >> On Wed, Feb 25, 2009 at 5:27 PM, Ted Kremenek <kremenek at apple.com>
>> >> wrote:
>> >>>
>> >>> On Feb 25, 2009, at 8:56 AM, Ben Laurie wrote:
>> >>>
>> >>>> I'm interested in looking at detecting "known bad" patterns, for
>> >>>> example:
>> >>>>
>> >>>> (<expr> & 0) == 0 (this example is stolen from FindBugs)
>> >
>> > Quick question.  Did you literally want to look for this syntax in the
>> > AST
>> > (i.e., the integer literal '0') or did you want to use the dataflow
>> > analysis
>> > to see cases when an expression evaluates to '0' and then is
>> > bitwise-ANDed
>> > with <expr>?  Is the motivation of this check is that it is trivially
>> > true?
>> >
>> >>>> if(<non-boolean value>) (cause of recent OpenSSL vuln)
>> >
>> > This seems a little too vague to me.  People check non-boolean values
>> > all
>> > the time in 'if' conditions.  Were you thinking about cases when <non
>> > boolean value> was negative?  Positive numbers seem completely legit to
>> > me
>> > (e.g., some flag was set in a bitmask, etc.).
>> >
>> > In either case, both of these seem like "auditor" checks.  For the first
>> > check if all you wanted to do is grep the syntax, we can easily add
>> > another
>> > check that gets called by AnalysisConsumer.  Check out the functions
>> > "ActionXXX" in AnalysisConsumer.cpp to see all the places where the
>> > various
>> > checks are called.  An ActionXXX can get called for each function, an
>> > entire
>> > Objective-C class, or the entire translation unit.  For example:
>> >
>> > static void ActionWarnDeadStores(AnalysisManager& mgr) {
>> >  if (LiveVariables* L = mgr.getLiveVariables()) {
>> >    BugReporter BR(mgr);
>> >    CheckDeadStores(*L, BR);
>> >  }
>> > }
>> >
>> > This action gets called for each function/method declaration.  The
>> > action
>> > queries the AnalysisManager (the thing that runs the checks) for the
>> > live
>> > variables analysis result on the current function (which gets computed
>> > on-the-fly).  The function 'CheckDeadStores' is then called with (a) the
>> > results of the live variables analysis and (b) and BugReporter object
>> > which
>> > is used to issue bug diagnostics to the user.  Analyses get registered
>> > in
>> > the Analyses.def file, where the "scope" of the analysis is
>> > declaratively
>> > specified.  For example:
>> >
>> > ANALYSIS(WarnDeadStores, "warn-dead-stores",
>> >         "Warn about stores to dead variables", Code)
>> >
>> > Here "Code" means "code declaration"; that is a FunctionDecl or an
>> > ObjCMethodDecl.
>> >
>> > A syntactic check can just walk the AST.  A good example of a syntactic
>> > check is CheckObjCDealloc.cpp.  Although this check will one day be
>> > revamped
>> > to do more real analysis, you can see how it walks the AST using
>> > iterators
>> > and does various pattern matching.  Syntactic checks are fairly easy to
>> > implement in this way and hook up to AnalysisConsumer.  The down-side is
>> > that they can be fairly dumb if the property you are checking requires
>> > any
>> > real smarts.
>> >
>> > Another possible way to implement an "auditor" is to use the GRAuditor
>> > interface defined in GRAuditor.h:
>> >
>> > template <typename STATE>
>> > class GRAuditor {
>> > public:
>> >  typedef ExplodedNode<STATE>       NodeTy;
>> >  typedef typename STATE::ManagerTy ManagerTy;
>> >
>> >  virtual ~GRAuditor() {}
>> >  virtual bool Audit(NodeTy* N, ManagerTy& M) = 0;
>> > };
>> >
>> > Checks that subclass GRAuditor<const GRState*> can be registered with a
>> > GRExprEngine and have their "Audit" method called after the path engine
>> > evaluates all expressions of a given type.  The Auditor is then free to
>> > inspect the analysis state at that point and flag errors.  A good
>> > example of
>> > this kind of check is in BasicObjCFoundationChecks.cpp.  The class in
>> > question is AuditCFNumberCreate, which flags misuses of the function
>> > CFNumberCreate
>> >
>> > (http://developer.apple.com/documentation/CoreFOundation/Reference/CFNumberRef/Reference/reference.html).
>> >  That function just interposes on the path engine at every CallExpr and
>> > checks the arguments in the function call for illegal values.
>> >
>> > This is probably enough for one email.  I'm more than happy to dig into
>> > more
>> > details depending on what direction you decide to go in.  Also, these
>> > interfaces are obviously evolving.  We haven't written many checks, so
>> > any
>> > idea of how to refactor the interface, make it better, etc., are
>> > welcome.
>>
>> I found some time to take a look at this today, and I'm puzzled: I
>> can't see where auditors choose what type they are interested in?
>
> Is this what you are looking for:
>
> GRExprEngineInternalChecks.cpp:470:
>     AddCheck(new CheckAttrNonNull(BR), Stmt::CallExprClass);
>

Ah, yes, thanks!