[cfe-commits] [Patch] Checker for assignment of non-Boolean value to Boolean variable

[Ted Kremenek]

Hi Ryan,

We've discussed this a bit offline, so the patch (and test cases) look good to me.  I think it is good enough for an initial check-in, but let's wait just a bit to see if there is any other feedback from the community.

The next step I'd like to see (after the initial commit of this checker) is to improve the diagnostics.  Here are some ways it can be improved:

(1) The diagnostic is too terse; it misses the punch line.  The issue is not that you are assigning a non-boolean value on its own, it is that you are assigning a non-boolean value to a location of boolean type.  I would also just stick with the generic term "boolean" as opposed to "Boolean" (which looks like a proper noun) if you aren't referring to a specific type.  Here's an example of what I mean:

  Assignment of a non-boolean value to a location of type 'Boolean'

The string in quotes should be automatically generated from the value type of the location being stored to.

(2) This might be overkill, but this could be further finessed to indicate the location of the store.  Sometimes this helps users.  For example:

  Assignment of a non-boolean value to variable of type 'Boolean'

  Assignment of a non-boolean value to field 'y' of type 'bool'

  Assignment of a non-boolean value to an array element type 'bool'

and so on.  Some other checkers do some of this.  It would be great if we could start refactoring some of this so more checkers could provide such improved diagnostics.  On the other hand, this could get overly verbose.  I'm only making this suggestion because users have requested this kind of refinement for other checkers in the past.

(3) It's worth indicating in the diagnostic that this is a security problem.  E.g:

  Assignment of a non-boolean value to a variable of type 'Boolean' (potentially insecure)

As a further refinement, including the CERT advisory # might be scary enough for some people to take the warning more seriously.

(4) "non-boolean" may be a bit too clinical.  It might be more helpful for users to just directly say what you mean, e.g.:

  Assignment of a non-boolean value (i.e., a value other than 0 or 1) to a variable of type 'Boolean' is potentially insecure

Of course the diagnostic is just getting longer, which is a concern, but I feel that this stuff can go a long way to helping users understand what the checker is trying to tell them.  You could also go completely crazy, but very specific:

  Assignment of a negative integer value to a variable of type 'Boolean' is potentially insecure
  Assignment of a value greater than 1 to a variable of type 'Boolean' is potentially insecure
  Assignment of a value greater than 1 ('42') to a variable of type 'Boolean' is potentially insecure

and so on.  This goes a long way to helping the user understand what they actually did wrong.


It would be interesting if we could explore having an analogy to "notes" in the static analyzer diagnostics, which could help break up such long diagnostics.  The issue is how would this integrate well with the path diagnostic scheme, with multiple diagnostics (events along a path) are already shown for an analyzer issue.

Thanks for working on this.

- Ted

On Dec 7, 2011, at 5:53 PM, Ryan Govostes wrote:

> Hello,
> 
> Please find attached a new checker for the static analyzer that warns on the assignment of a non-Boolean value to a variable with a Boolean data type. It supports Boolean data types defined by C++, C99, and Objective-C as well as those in stdbool.h and MacTypes.h.
> 
> Thanks,
> Ryan Govostes
> 
> <BoolAssignmentChecker.diff>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits