[webkit-dev] Eliminate potential null pointer dereference?

[Maciej Stachowiak]

On Apr 20, 2012, at 1:48 PM, Rachel Blum wrote:

> I completely agree with Maciej here that if this is a reachable code, then the patch author should put a reasonable effort into creating a test case. And most importantly, these changes are clearly not "code cleanup".
> 
> I'm disagreeing here. (as far as NULL checks go).
> 
> Unless there's a demonstrable reason that you _need_ a value uninitialized, why is the burden of proof on the person doing cleanup? Yes, at the point the code was written, it's well possible that the author was aware that the value would always be initialized for use. However, if code is added to a class, that invariant is not always checked again.

I think there's a difference between null checks and initializing variables. I think it would be a reasonable style guideline to say that a constructor must initialize every data member, except possibly ones that have their own constructors or cases where the reason not to initialize is documented.

I don't know if I'd agree with a style guideline that says "add a null check everywhere that it seems like a value might be null".

> 
> I think the confusion is over the intent of the person making the cleanup change. We (I speak as one of the people pushing static analysis) are not interested in *changing* WebKit behavior. We're interested in making sure behavior is deterministic. Requiring the construction of what amounts to an exploit for each fix for uninitialized variables seems a bit overkill :)
> 
> I agree that the CHANGELOG entry should state that we deliberately didn't add tests. My personal policy is to propose those patches, complete with "No new tests/ cleanup only". If I get pushback on the review, I'm happy to abandon it. 

I think given our current project policies, the best practice would be:

- Identify that the change was made in response to a static analyzer (and identify the tool)
- If the patch would cause a behavior change, make a reasonable attempt to make a test
- If the code path is in practice unreachable, then document that, and also think about whether the way it's addressed makes that clear.
- If you can't determine either way, then document that. "I don't know of a way to reach this code path but I can't prove it's impossible" is more accurate than "No new tests/cleanup only".

You mentioned in another email that you're not in favor of blindly doing everything the tool says, so I think that the above is a reasonable thing to expect as part of thinking about the tool's output.

To take the example of the last patch here, if the variable in question can't actually be null on exit from the first loop, the right fix would have been to change the loop condition rather than to add a null check. That would have made the code more readable for future programmers. If it is reachable, then the test would have helped people who do not have access to the relevant analysis tool, or the same level of understanding about which of its messages to obey and which to ignore. So I think the expectations here are reasonable.

Regards,
Maciej

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20120420/1e2bccde/attachment-0001.html>