[webkit-dev] Eliminate potential null pointer dereference?

[Maciej Stachowiak]

Hi Luke,

I feel like you've been put on the defensive a bit here, which is unfortunate. I do agree with the value of readable code and "hackability" is one of the core values of the WebKit project. Code should indeed be meaningful to programmers. One thing to keep in mind though is that, by this definition, the standard of clean code is partly subjective. If other developers on the project do not in fact find changes to be more readable, then it's not actually a readability improvement.

As far as regression tests vs. static analyzers: the goal of regression tests is not only to prevent bugs from recurring but to have a shared way for the community as a whole to do so. A static analyzer might be an adequate substitute in some cases if others (a) knew about it and (b) agreed that it should become one of the project's testing tools. In other words, it's not the form of the test that's key, it's the knowledge and general availability of the relevant testing tool.

Finally, one additional comment on another email of yours:

On Apr 20, 2012, at 11:25 AM, Luke Macpherson wrote:
> On Fri, Apr 20, 2012 at 11:07 AM, Ryosuke Niwa <rniwa at webkit.org> wrote:
>> Is the code reachable? It's quite possible that the code is unreachable and
>> therefore there is no way to hit that crash. Without a test, we can't answer
>> that question.
> 
> That is not rationally true. A test case can show that there is a code
> path leading to a null pointer dereference. A test cannot show that
> there are no possible code paths that lead to that state. This is
> exactly what I was getting at when explaining that the state space of
> webkit is too large to test. In this case we don't have a repro case
> that leads to that state, but that does not mean that it is not
> possible, or that the potential to crash should not be fixed.

The WebKit project's regression test policy does not require you to prove a universal negative, just to make a test that provides reasonable coverage, would have failed without the change, and passes with it. Historically as a project, we have not chosen to give up on testing even though perfect testing is infeasible.

Regards,
Maciej

On Apr 20, 2012, at 10:53 AM, Luke Macpherson wrote:

> Changing the two loops to use the same form improves readability
> because it makes it clear that the form of iteration is the same
> between the two loops. This is a very common C pattern when dealing
> with lists where the behavior changes after an element is encountered.
> The pattern is used instead of a flag because it eliminates branches
> in the code. In this case, not using the canonical two-for-loop form
> has lead the original writer to create code that is not obviously
> correct - you can’t look at this function and see that it does what it
> claims - in fact, the most obvious code path of exiting the first loop
> through the while condition directly results in a null pointer
> dereference.
> 
> Code should communicate meaning to other software engineers. It should
> allow us to reason together about the code, and to have certainty
> about what the code will do. I don’t think anyone could look at the
> existing code and be assured that it was correct. Fixing that problem
> and clearly communicating the correctness of the code is not “code
> churn” - on the contrary it is the kind of change that is vital to the
> long-term health of the codebase.
> 
> Tests are a good thing, but they are not the only thing. Consider the
> state-space of a large piece of software like webkit - it is
> essentially infinite. You can’t test every case and code path to
> ensure correctness. On the other hand, we don’t yet have the tools to
> produce a proof of correctness for webkit, and so we live somewhere
> between the two - some of our confidence comes from being able to
> reason about the correctness of the code in general, and some of our
> confidence comes from being able to test a small fraction of the state
> space.
> 
> What do we hope to achieve by adding a test when fixing a bug? To
> prevent a bug from being reintroduced into the codebase at a later
> date. This is a reasonable goal, so let’s remember that the goal is to
> prevent the bug from recurring, not to add a test for its own sake. In
> this case, the potential null pointer dereference was found using
> coverity, a static analysis tool that we run nightly. If the bug were
> to be reintroduced it is reasonable to expect that static analysis
> would be able to find it again.
> 
> Hope this helps you see where I'm coming from,
> Luke
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev