[webkit-dev] Eliminate potential null pointer dereference?

[Luke Macpherson]

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