Unit Testing Uncovers Bugs

As part of the ‘utility’ library in one of the projects we are using at work, I wrote two small wrappers around strtol() and strtoul(). These two functions support a much more useful error reporting mechanism than the plain atoi() and atol() functions, but getting the error checking right in all the places they are called is a bit boring and cumbersome. This is probably part of the reason why there are still programs out there that use atoi() and atol().

For example here’s how I usually check for errors in calls to the strtol() and strtoul() functions:

char *endp;
long x;

endp = NULL;
errno = 0;
x = strtol(str, &endp, base);
if (errno != 0 || (endp != NULL && *endp != '\0' &&
    (isdigit(*endp) != 0 || isspace(*endp) == 0)))
        /* Return 'endp' if possible. */
        return -1;
}
/* At this point 'x' contains the parsed value. */

This is a lot of code for parsing a single long value. For one or two input strings it may be ok to repeat the code in the places where the numeric parsing code is needed. For more than a couple of input strings it really feels boring to repeat this code again and again.

When I set out to write the wrapper code for strtol() and strtoul() my goal was to make it very easy to parse input strings. A typical call to the parsing function should be a single line of code; it should be very clear if the parsing attempt succeeded or failed; it should also be possible to get both the parsing success or failure and the numeric value we just parsed; it should also be possible to get hold of the last character we managed to parse, so that strings like “100 200 300″ can be parsed efficiently without having to manually find where the textual representation of the first number ends or the second one starts.

That’s quite a list of goals for a single function, but the function call style I envisioned looked something like this:

long value;
char *endp = NULL;

if (parselong("0x12345678", &endp, 16, &value) != 0) {
        err(1, "parse error");
}

The return value of parselong() makes it very clear if the parsing attempt succeeded or failed. A return value of zero means success. Any other return value means failure.

The parsed value is returned through the &value pointer. If the parsing attempt has failed parselong() can leave the value unmodified to avoid inflicting spurious side-effects to its calling code because of a failed attempt to parse an input string.

If the parsing attempt has succeeded, &endp may be set to point right after the last character that was successfully parsed. This is actually part of the documented interface of strtol() and strtoul(), so it comes for free by wrapping these functions.

Finally, parsing a long value is a single function call. It is a lot easier to call the parsing function without having to repeat all the error checking boilerplate at each calling site. It’s even easy to “chain” multiple parsing attempts using a style similar to:

long value1, value2, value3;

if (parselong("0x12345678", NULL, 16, &value1) != 0 ||
    parselong("0xdeadbeef", NULL, 16, &value2) != 0 ||
    parselong("0xf00fc0de", NULL, 16, &value3) != 0)
        err(1, "parse error");

Not that this is a good style of reporting errors, but it is possible, just because it’s now easy to parse a value and check if it was parsed correctly with a single line of code.

The Unit Tests Fail on Linux

Several months passed after I wrote the initial parselong() and parseulong() functions. In the meantime I had to port the program using them to other platforms. The initial target platform was FreeBSD.

This is a bug that lurked for a few months in the initial code of parselong() until I had to port the function to another platform and started writing unit tests to verify that it works the way I expected it to work on all possible systems. In retrospect I should have started by writing the unit tests, but that’s something I can say now because I finally got around to doing it and they did serve a very useful purpose.

When I had to port my ‘utility’ functions to work on several Linux versions too, I wrote a collection of unit tests for parselong() and parseulong(). The testing framework I used was CUnit because of the way it nicely integrates with plain ANSI C code.

One of the test functions I wrote was supposed to check for failures returned by parselong() for invalid input strings. The bulk of the test function was something like this:

#include "CUnit/Basic.h"

void
test_parselong_failures(void)
{
        long value = TEST_VALUE_ULONG_MAGIC;

        CU_ASSERT_EQUAL(parselong("xxx", NULL, 0, &value), -1);
        CU_ASSERT_EQUAL(value, TEST_VALUE_ULONG_MAGIC);

        CU_ASSERT_EQUAL(parselong("+", NULL, 0, &value), -1);
        CU_ASSERT_EQUAL(value, TEST_VALUE_ULONG_MAGIC);

        CU_ASSERT_EQUAL(parselong("-", NULL, 0, &value), -1);
        CU_ASSERT_EQUAL(value, TEST_VALUE_ULONG_MAGIC);
        ...
        CU_PASS("parselong() failures for invalid values look ok");
}

Running the unit tests on FreeBSD seemed to work fine. After all the initial version of the parselong() function had been manually tested with the same input strings earlier.

When I tried running the same test cases on Linux though, they failed. Apparently parselong() was not detecting that strtol() failed to parse the input string “xxx” or any other input strings from the ones tested in the test_parselong_failures() function!

The Bug Uncovered

Adding a couple of debugging printf() calls to parselong() itself showed that on Linux parselong() was returning zero for invalid input strings when strtol() could parse no character at all from the input string.

The initial version of the error checking code for strtol() was similar to:

char *endp;
long x;

endp = NULL;
errno = 0;
x = strtol(str, &endp, base);
if (errno != 0 || (endp != NULL && endp != str && *endp != '\0' &&
    (isdigit(*endp) != 0 || isspace(*endp) == 0)))
        /* Return 'endp' if possible. */
        return -1;
}
/* At this point 'x' contains the parsed value. */

The highlighted part (endp != str) of the error checking code assumes that strtol() will move the ‘endp’ pointer at least one character after the start of the input string. Apparently on Linux this is not the case. The strtol() function of Linux does not move ‘endp’ at all if it cannot parse even a single character of the input string. This seems to be the correct behavior for strtol(), but it was hidden for a while, lurking in the original parselong() code, until I ran the unit tests of the function on Debian GNU/Linux.

The CUnit driver program that I used to run the test cases failed on Linux with error messages like:

  1. test_parselong.c:63  - CU_ASSERT_EQUAL(parselong("xxx", NULL, 0, &value),-1)
  2. test_parselong.c:64  - CU_ASSERT_EQUAL(value, TEST_VALUE_ULONG_MAGIC)
  3. test_parselong.c:66  - CU_ASSERT_EQUAL(parselong("+", NULL, 0, &value), -1)
  4. test_parselong.c:67  - CU_ASSERT_EQUAL(value, TEST_VALUE_ULONG_MAGIC)

The culprit for these test case failures was the assumption that Linux would set errno to a non-zero value for an invalid input string… Apparently, it doesn’t. The following small program prints different output on BSD vs. Linux:

$ cat -n strtest.c
     1  #include <errno.h>
     2  #include <limits.h>
     3  #include <stdio.h>
     4  #include <stdlib.h>
     5
     6  int
     7  main(void)
     8  {
     9          long value;
    10          const char *input = "xxx";
    11          char *endp = NULL;
    12
    13          errno = 0;
    14          value = strtol(input, &endp, 0);
    15          printf("str = %p = \"%s\"\n", input, input);
    16          printf("endp = %p \"%s\"\n", endp, endp ? endp : "(null)");
    17          if (endp != NULL) {
    18                  printf("endp[0] = '%c' (%d 0%03o #x%02x)\n",
    19                    *endp, *endp, *endp, *endp);
    20          }
    21          printf("errno = %d\n", errno);
    22          printf("value = %ld 0%lo #x%lx\n", value, value, value);
    23          return EXIT_SUCCESS;
    24  }

On FreeBSD the output of this program includes an errno value of EINVAL:

freebsd$ cc strtest.c
freebsd$ ./a.out
str = 0x8048604 = "xxx"
endp = 0x8048604 "xxx"
endp[0] = 'x' (120 0170 #x78)
errno = 22
value = 0 00 #x0
freebsd$ fgrep 22 /usr/include/sys/errno.h
#define EINVAL          22              /* Invalid argument */
freebsd$

On a recent update of Debian GNU/Linux “testing” the output is slightly different:

debian$ cc strtest.c
debian$ ./a.out
str = 0x8048630 = "xxx"
endp = 0x8048630 "xxx"
endp[0] = 'x' (120 0170 #x78)
errno = 0
value = 0 00 #x0
debian$

This means that the only indication we have that the Linux version of strtol() failed to parse some of the input text is the value of ‘endp': it’s the same as the input string. The error-checking code of the original parselong() wrapper was:

        x = strtol(str, &endp, base);
        if (errno != 0 || (endp != NULL && endp != str && *endp != '\0' &&
            (isdigit(*endp) != 0 || isspace(*endp) == 0)))
                error(...);

But on Linux both of the following are true:

  • errno is not set to a non-zero value.
  • If strtol() could not parse even one input character, endp == str.

This caused parselong() to bypass the error checking code, and try to return a ‘valid’ result even tough the Linux strtol() version has failed. Hence the failure of the unit tests.

Removing the (endp != str) conditional expression means that the error checking code works equally well on Linux and BSD. The BSD version of strtol() returns a non-zero errno value, triggerring the first part of the error checking code. The Linux version returns an endp pointer that is non-null and fails the ‘\0′ check later on. The new parselong() function is slightly shorter and it passes the unit tests on both BSD and Linux.

Conclusions

There is something thrilling about fixing bugs by removing code. This bug was one of the few cases I’ve come across during the last couple of months where removing code was an improvement. There’s probably a joke about “writing too much code” and the bug-resolving debt each line of new code introduces. I think I’ll leave that for another time though.

The most important conclusion of today’s bug hunting session was that Unit Testing really does work and it pays back in real, quite tangible ways. Had I not spent a bit of time to think about what the parselong() and parseulong() functions are supposed to do, when they are supposed to fail and how they are allowed to fail, I would not spent the time to write test cases for them. Had I not written the test cases, I wouldn’t notice there is a failing test case on Linux. Had I not seen that I wouldn’t realize some times the two functions were returning completely bogus results on Linux systems.

The central place the unit testing code has in this story is an important and serious lesson for me:

KEEP TESTING!
About these ads
This entry was posted in Computers, FreeBSD, GNU/Linux, Linux, Programming, Software and tagged , , , , , , , . Bookmark the permalink.

13 Responses to Unit Testing Uncovers Bugs

  1. argp says:

    I really enjoyed this post George. Unit testing is indeed fundamental to any medium-to-large project. I always start with coding unit tests (it also helps a lot to clear up the internal, and of course the external API, in your mind and its relation, or lack thereof, to the design) in any project I plan to commit measurable time and effort.

  2. vvas says:

    So if even the first character of the string is invalid, and the FreeBSD strtol() does not set endp to be equal to str, what *does* it do, and why is that not a bug?

    • keramida says:

      vvas: FreeBSD’s strtol() does set endp to be equal to str (see the minimal BSD vs. Linux program whose output was used to compare behaviors). But FreeBSD also sets errno to EINVAL, which Linux doesn’t. So there’s no bug, you just get one extra indication about the error on BSD.

  3. 762e5e74 says:

    Some ‘testing’ fortune for you:

    A sheet of paper crossed my desk the other day and as I read it,
    realization of a basic truth came over me. So simple! So obvious we couldn’t
    see it. John Knivlen, Chairman of Polamar Repeater Club, an amateur radio
    group, had discovered how IC circuits work. He says that smoke is the thing
    that makes ICs work because every time you let the smoke out of an IC circuit,
    it stops working. He claims to have verified this with thorough testing.
    I was flabbergasted! Of course! Smoke makes all things electrical
    work. Remember the last time smoke escaped from your Lucas voltage regulator
    Didn’t it quit working? I sat and smiled like an idiot as more of the truth
    dawned. It’s the wiring harness that carries the smoke from one device to
    another in your Mini, MG or Jag. And when the harness springs a leak, it lets
    the smoke out of everything at once, and then nothing works. The starter motor
    requires large quantities of smoke to operate properly, and that’s why the wire
    going to it is so large.
    Feeling very smug, I continued to expand my hypothesis. Why are Lucas
    electronics more likely to leak than say Bosch? Hmmm… Aha!!! Lucas is
    British, and all things British leak! British convertible tops leak water,
    British engines leak oil, British displacer units leak hydrostatic fluid, and
    I might add Brititsh tires leak air, and the British defense unit leaks
    secrets… so naturally British electronics leak smoke.
    — Jack Banton, PCC Automotive Electrical School

    [Ummm … IC circuits? Integrated circuit circuits?]

    A novice asked the Master: “Here is a programmer that never designs,
    documents, or tests his programs. Yet all who know him consider him one of
    the best programmers in the world. Why is this?”
    The Master replies: “That programmer has mastered the Tao. He has
    gone beyond the need for design; he does not become angry when the system
    crashes, but accepts the universe without concern. He has gone beyond the
    need for documentation; he no longer cares if anyone else sees his code. He
    has gone beyond the need for testing; each of his programs are perfect within
    themselves, serene and elegant, their purpose self-evident. Truly, he has
    entered the mystery of the Tao.”
    — Geoffrey James, “The Tao of Programming”

    BtW, how’re you doing ? Real busy these days :)

  4. anonymous says:

    Keep Posting.

    Good stuph.

  5. Olga says:

    Hello George, my name is Olga, I represent BSD Magazine.
    Please contact me, I have proposition for you. +)

  6. Great piece of Info George! The idea of having bug free softwares has got me into Unit Testing field, I am sure i am going to learn a lot following your blog. Keep up the good work mate.

    • keramida says:

      Thanks for the good words. I’ll keep posting.

      Note, however, that I’m not affiliated in any way with TypeMock, so I don’t know if using their products can help one produce “bug free software”.

  7. Unit Testing says:

    Unit testing approach is really helpful for the developers as they can go on the right path by using this testing approach. it will be helpful, saves time and makes our code worth for the long time. Thanks to this blog for giving me helpful information related to the unit testing approach.

Comments are closed.