A Case for Static Analysis

I recently was browsing some code on GitHub when I came across a strange source file. From what I could read, there was a glaring bug in the code. It wouldn’t mean that the code didn’t work, merely that it wouldn’t work as expected (or documented).

What was strange though was how small the file was (barely more than a screen) and how clearly the bug stood out. Surely it should have been caught, I thought.

The C Code

After a bit of getting re-acquainted with C (I’d forgotten that you needed to define functions before their first use in the file!)¹ I mocked up this approximation of the code I saw in the wild:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32

#include <stdio.h>
#include <unistd.h>

char *SSID = "MY_EPIC_WIFI";
char *PASS = "MELLON";

void wifi_begin(char *ssid, char *pass) { }

char is_connected() { return 0; }

/**
 * @note usual wait time is max 23 seconds for fail
 */
int main () {
    wifi_begin(SSID, PASS);

    sleep(3);

    printf("Connecting to wifi.\n");
    int cnt = 0;
    while (!is_connected()) {
        if (cnt > 20)
            printf("\n");
            return 1;
        sleep(1);
        ++cnt;
        printf(".");
    }

    printf("\n");
    return 0;
}

Did you catch it? To its credit, gcc did:

error.c: In function ‘main’:
error.c:23:9: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
         if (cnt > 20)
         ^~
error.c:25:13: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’
             return 1;
             ^~~~~~

This does require you to pass -Wall though (or at least -Wmisleading-indentation). Lets zoom in on those lines:

while (!is_connected()) {
    if (cnt > 20)
        printf("\n");
        return 1;
    sleep(1);

Now it becomes clear what has happened - someone added some print statements to do debugging and didn’t guard the if statement properly. The correct solution is, of course

while (!is_connected()) {
    if (cnt > 20) {
        printf("\n");
        return 1;
    }
    sleep(1);

Tragically, the “fix” that was applied in a commit titled "-Wall no longer fails compile." was

while (!is_connected()) {
    if (cnt > 20)
        printf("\n");
    return 1;
    sleep(1);

This is a case of someone making a lot of changes (230+/241-) to fix a problem without carefully considering each change. Most were truly white-space or parenthesis issues but this one snuck under the wire.

The root issue is of course that lines 25 to 27 never run and the loop isn’t a loop. This is something that a static analyser would catch, but unfortunately gcc didn’t pick up on that².

The Rust Code

Now lets turn my current language of choice on the problem. If we copy the code line by line we hit an immediate problem

error: expected `{`, found `print`
  --> error.rs:24:13
   |
23 |         if (cnt > 20)
   |         -- this `if` expression has a condition, but no block
24 |             print!("\n");
   |             ^^^^^--------
   |             |
   |             expected `{`
   |             help: try placing this code inside a block: `{ print!("\n"); }`

Rust won’t even let you have a bare if statement. At this point the question is rather moot since adding the braces gives this obviously wrong code³:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30

use std::thread::sleep;
use std::time::Duration;

const SSID : &str = "MY_EPIC_WIFI";
const PASS : &str = "MELLON";

fn wifi_begin(_ssid : &str, _pass : &str) { }

fn is_connected() -> bool { false }

fn main() -> Result<(), &'static str> {
    wifi_begin(SSID, PASS);

    sleep(Duration::from_secs(3));

    print!("Connecting to wifi.\n");
    let cnt = 0;
    while !is_connected() {
        if cnt > 20 {
            print!("\n");
        }
            return Err("Connection failed");
        sleep(Duration::from_secs(1));
        cnt += 1;
        print!(".");
    }

    print!("\n");
    Ok(())
}

However, more to the point, this gives the warning

warning: unreachable statement
  --> error.rs:23:9
   |
22 |             return Err("Connection failed");
   |             ------------------------------- any code following this expression is unreachable
23 |         sleep(Duration::from_secs(1));
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unreachable statement
   |
   = note: `#[warn(unreachable_code)]` on by default

warning: 1 warning emitted

and that is not fixable by changing whitespace.

A Fair Fight?

Proponents of rust (and there are many - vocal ones too) often laud its static analysis as a great strength of the language⁴. Here that claim rings true. C, on the other hand, is not known for its static analysis and analysers and linters are not built into the toolchain.

No doubt there is some tool that run against the C code above would highlight the error. However, with the language as old as it is, one might well hope that it would be built in by now. So I’d say that, yes, this was a fair comparison.

One should note though that this example is minor compared to something as powerful as lifetimes. When rust fans talk about static analysis, inevitably they are actually talking about the borrow checker and friends. This is a whole other ball game though and not really the point of this post.

At the end of the day, most bugs are going to be human oversight. The best we can hope for is to set up checks and balances against our own folly.