Vigilant Software Blog

Are you interested in code quality? We're starting a new series of blogs called Defect Spotlight. In it, we'll be bringing you a new example of a real-world defect found by static analysis. Learn from the mistakes shown here!

I think it's safe to say we love software testing. We test our projects with a variety of approaches, both static and dynamic. We use a combination of testing methods because they're not all created equal; they each have their strengths and weaknesses. As a software engineer or tester, it pays to be aware of these. One well-known problem with dynamic testing is the difficulty of reaching every path through a program. Often, these are the error handling paths, or otherwise infrequently visited paths in your code.

Since most programs work reasonably well under normal conditions, static analysis usually reports problems in the corner cases, error-handling paths, and other areas that haven't been adequately tested through other approaches.

This week's example is from git, the popular distributed revision control system. One thing to note in the snippet below is that the error() function does not call exit(3) or similar, so it is not an exit point for the function. Given that hint, see if you recognize the error in this example:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20

f = fopen(file, "w");
if (!f)
    error("Unable to open marks file %s for writing.", file);

for (i = 0; i < idnums.size; i++) {
    if (deco->base && deco->base->type == 1) {
        mark = ptr_to_mark(deco->decoration);
        if (fprintf(f, ":%"PRIu32" %s\n", mark,
                sha1_to_hex(deco->base->sha1)) < 0) {
            e = 1;
            break;
        }
    }
    deco++;
}

e |= ferror(f);
e |= fclose(f);
if (e)
    error("Unable to write marks file %s.", file);

This snippet is from git's export_marks function. The file handle f is opened on line 1 and tested for NULL, and in the case that it is NULL, an error message is printed to the console. However, the function does not exit at this point, it keeps running. Depending on the path taken, control will reach either the fprintf on line 8 or the ferror call on line 17 using the invalid file handle f, causing a crash.

In this case, the crash is easy to reproduce: attempt to export marks into a directory where you don't have write permission:

[~/some_git_repos]$  git fast-export --export-marks=/foo
error: Unable to open marks file /foo for writing
Segmentation fault (core dumped)

This isn't unique to git; these types of untested error-handling paths exist in virtually every program we've analyzed. It just shows that there's a lot to be gained from having a well-rounded approach to testing, using all the tools that are available.