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

[David Blaikie]

On Thu, Dec 8, 2011 at 7:23 AM, Ted Kremenek <kremenek at apple.com> wrote:
> 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.

Apart from your mention downthread that notes in the clang sense
aren't currently applicable to the SA I would've thought a note
pointing to the declaration of the variable/field/array would be
helpful here - though I suppose that's as complicated, if not moreso,
than switching the text in the original diagnostic as you showed (once
you have to account for assignments to expressions that aren't named
variables - such as a function call that returns a reference).

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

Indeed, to be honest at first blush I wasn't sure of the point of this
diagnostic. MSVC likes to warn about 'performance' issues when
implicitly converting from int->bool & that usually just annoys me (
http://msdn.microsoft.com/en-us/library/b6801kcy.aspx ) rather than
providing anything terribly useful.

What's the actual security issue at stake here?

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

Is there any current (or future planned) effort to have extended
documentation for each diagnostic/checker that might describe in
detail the issues & solutions (something like MSDN but a little more
useful, perhaps)? (as much as it's good to have the diagnostic be self
descriptive & actionable, it's sometimes hard to convince people that
what they see as a legitimate construct is actually dangerous (take,
for example, Chris's blog posts on undefined behavior - even some
people who know what UB is still like to think that "oh, it invokes UB
but the compiler will never do anything but <x>"))

- David

>
> 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
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits