Sunday, November 8, 2015

VerySillyMUD: Understanding a Function with Tests

This post is part of my "VerySillyMUD" series, chronicling an attempt to refactor an old MUD into working condition[1].

In our last episode, we got a basic framework for unit tests in place. Just to refresh our context (and which layer of yak we are shaving at the moment): we need unit tests so we can (a) capture the current behavior of a particular function, dsearch(), so that we can (b) refactor it safely, so that we can (c) fix a compiler warning about it, so that we can (d) finish fixing all the compiler warnings in case there are other latent bugs being hidden. Whew!

So let's take a look again at the function in question:

What can we figure out about it? Well, first, it seems to take two string arguments, helpfully named "string" and "tmp". It seems like both are potentially modified by the function here; tmp gets written on line 11 via strcpy(), and string gets written on line 24 via sprintf(). The i variable seems to be a flag indicating whether we're done, as the main while loop tests for it and the only modification to it is in line 10 where it is set to 1. So let's start with the simplest case, where we only go through the loop once; in order for that to be true, we need the call to strchr in line 9 to find no instances of the tilde ('~') character in string. In this case, it looks like we just copy string into tmp and are done, which suggests that our first unit test can be:

So let's try to run that:

Now, what about a more complicated test? It seems like tmp might be the output variable here, so let's see what happens when we feed it a string with a tilde in it, by writing an intentionally failing test:

And then run the test:

Hmm, a little less than I was hoping for; the Criterion library macro-interpolates the C source expression into the error message. In the case of string comparison, I'd rather have seen the string values here instead. At this juncture I took a quick browse of the Criterion source code to see if this was a quick fix, but it wasn't, so I decided to pass on this for now and just fix it by doing a good ol' printf to see what the value was. In this case, it looks like the behavior is to strip out the tilde and the following space, because this test passes:

A quick perusal of the source shows we must be in the default: clause of the switch statement. Now, like any good tester, let's see what happens if we have a tilde as the last character:

Ah, cool! That one passes. So now let's look at some of the other cases, starting with a "~N". I'm going to guess that this substitutes the string "$n;" in for "~N":

Indeed, this passes. Similarly, it looks like "~H" will turn into "$s". (It does, although I won't show that test here just for brevity). Ok, what should we test next? How about two tildes in a row? I think that this should just get stripped out because the second tilde won't be either 'N' or 'H':

Yep. Ok, now we have a loop, so this suggests we should be able to do multiple substitutions:

Check. That would seem to cover most of the output behavior on the second argument to dsearch(), which is labeled tmp. As we noted, though, the first parameter, string sometimes also gets written, in the sprintf on line 24. So we had better document its behavior too. However, notice that sprintf(string,tmp) essentially copies tmp into string, as long as there are no format expansions like %d in tmp, and if there are, then we don't have any arguments for them! So this is likely a bug, especially if string comes from an untrusted input source. Now, if you remember a couple episodes ago, this was the exact compiler warning we ran into:

Ok, so this means that we should pretty much just add tests that show that string has the same output behavior as tmp, and then I think we can refactor it.

At this point I feel pretty confident I understand what this function does. Let's rewrite it more simply. What we'll do is we'll build up the substituted string in tmp and then copy it back into string at the end:

Sure enough, the tests pass, so we'll call this a victory for now. In the next episode, we'll turn the -Werror flag back on and continue fixing compiler warnings as we were trying to do previously. Fortunately, we now have the beginnings of some unit tests we can use if we encounter anything that doesn't look like it has an immediately obvious fix.


[1] SillyMUD was a derivative of DikuMUD, which was originally created by Sebastian Hammer, Michael Seifert, Hans Henrik Stærfeldt, Tom Madsen, and Katja Nyboe.

0 comments: