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!

Most of what we do revolves around finding software defects. They're everywhere, so it's not usually hard to find them. One thing we have learned is that Sentry spots not only the hard-to-find defects, but also the ones hiding in plain sight.

We've been analyzing many open source C and C++ programs on a nightly basis. As we continue growing the list of projects we inspect, we hope to help improve the quality and security of open source software.

When we add a new project, we tend to ignore all of the existing defects Sentry reports. This isn't to say that Sentry didn't find a defect worth reporting to the project, it most likely did. The issue is, developers care much less about old defects than they do about new ones. Especially brand new ones, like the ones committed last night.

In fact, in our experience, this is so important that reporting bugs immediately is the key factor in motivating developers to fix them. When someone is actively working on the code in question, it's generally easier (and safer) to fix a bug than it would be several months later.

Recently, we added Audacity to the list of C++ projects that we analyze nightly. Sentry found 305 potential defects. I casually reviewed some of the memory leaks reported by Sentry and found one that was pretty blatant. I decided in this instance to report this old issue.

 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
33
34
35
36
37

double AudacityProject::NearestZeroCrossing(double t0)
{
   int windowSize = (int)(GetRate() / 100);
   /* [Variable dist allocated but never returned or deleted.] */
   float *dist = new float[windowSize];
   int i, j;

   for(i=0; i<windowSize; i++)
      dist[i] = 0.0;

   TrackListIterator iter(mTracks);
   Track *track = iter.First();
   while (track) {
      /* [Code that doesn't touch dist snipped for brevity] */
      for(i=0; i<windowSize; i++) {
         if (windowSize != oneWindowSize)
            j = i * (oneWindowSize-1) / (windowSize-1);
         else
            j = i;

         dist[i] += oneDist[j];
      }

      track = iter.Next();
   }

   int argmin = windowSize/2; // Start at default pos in center
   float min = dist[argmin];
   for(i=0; i<windowSize; i++) {
      if (dist[i] < min) {
         argmin = i;
         min = dist[i];
      }
   }

   return t0 + (argmin - windowSize/2)/GetRate();
}

The memory leak that Sentry found was committed here. Take a look at the function NearestZeroCrossing above. It allocates the variable dist with the new[] operator, but never returns or deletes it, so the associated memory will always be leaked. In this case, the Audacity team was happy to accept a patch to fix it.

The interesting thing to note here is not that Sentry found a memory leak, it's that we found and fixed a memory leak that was eight years old.

Software does not fix itself. It takes good programmers to improve software. Even then, software defects are not always easy to spot, especially when the program appears to run well. Unit, integration and system testing are great ways to shake out defects and ensure software behaves correctly. Static analysis makes a great addition, helping you spot those hard to find bugs that don't occur on the happy path. Or, as in this example, it can find those eight-year-old bugs hiding in plain sight.