Vigilant Software Blog

Are you interested in code quality? This post is part of a series called Defect Spotlight. In it, we'll be bringing you instructive examples of real-world defects found by static analysis. Learn from the mistakes shown here!

One of the advantages of a statically-compiled language like C or C++ is that a good deal of information is available at compile time. While robust software can be written in more dynamic languages, it requires a certain level of attention to detail and a very thorough suite of dynamic tests.

This extra compile-time information is often a blessing for catching bugs early. First of all, your compiler catches a number of stupid mistakes before you even get a chance to run any tests, from misspelled names to using incorrect data types.

We added a couple checks recently for simple mistakes like checking an unsigned` value for being less than zero, which is clearly impossible. You'd never do that, right? Of course, experience has taught us that there is no mistake that's too simple to make. Even the best engineers make simple mistakes on a fairly regular basis.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10

len = g_socket_receive_message (nl->priv->sock, NULL, &iv, 1,
                                NULL, NULL, &flags, NULL, &error);
if (len < 0)
  {
    g_warning ("Error on netlink socket: %s", error->message);
    g_error_free (error);
    if (nl->priv->dump_networks)
      finish_dump (nl);
    return FALSE;
  }

With the new scanner, Sentry found this perfectly innocent-looking code in GLib. If g_socket_receive_message returns a value less than zero, the error-handling logic will clean up and return, right? Unforunately, this code is unreachable because len was declared to be an unsigned type (gsize), and can never have a value less than zero. The check is useless because the condition is provably false at compile time. (Sentry also checks for checks that are useless because they will always be true — the else path could never be reached.)

When this code encountered a network I/O problem, it most likely would have caused undefined behavior in the program. The structure would not have been properly initialized and the code would've continued to execute as if it had been. Not only that, but it's the insidious kind of dead (unreachable) code that appears to be reachable. Debugging the problem could have taken hours trying to determine why the structure is (only sometimes) full of garbage. (We reported this bug to the GLib developers, and it was quickly fixed.)

It's a good reminder that automated checks for even very simple mistakes can produce valuable feedback. Taking advantage of the additional type information we have at compile time can save programmer hours and prevent entire classes of defects from impacting you in production.