Friday, November 20, 2015
VerySillyMUD: Adding autotools

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 all the remaining compiler warnings fixed. While I was merging the pull request for it, I noticed GitHub was prompting me to get continuous integration set up. I had run across Travis CI before, and knew that it was free to use for open source projects. A quick peek around their documentation shows that they support C, but that they assume the use of GNU autotools for building. Since a friend had already identified weirdness from the runs of makedepend I had done on my own computer and checked in, I actually already had an issue open for this. Seems like the universe is trying to tell me something!

Conveniently, autotools comes with a sample project and a pretty good step-by-step tutorial. We also have a working Makefile that we can use for reference--for now I'm just going to make a temporary copy of it as Makefile.orig so that I can have it for easy reference, and then clean it up later during commit/PR structuring. Since automake is going to overwrite the Makefile, this will be convenient, even though I know a copy of the Makefile is safely tucked away in version control. Ok, let's start with the toplevel, which for now just has to point to the src/ directory:

Then we need another for the src/ directory. In this case, it looks like the bare minimum is to identify the final executable name and then identify the .c source files. Not sure if we need to add the .h ones or not yet; it could be that autoconf will find those later. Anyway, let's try this:

As for the in the source directory, we can adapt the sample one from here and try this:

Now, per the instructions, we're supposed to run "autoreconf --install":

Hmm, I had thought that all the CFLAGS would go in the AM_INIT_AUTOMAKE macro, but I guess not. Let's just put -Werror in there for now and try again:

Ok, this is much closer. Looks like we just have some missing files. For now, I'll create empty versions of NEWS, README, AUTHORS, and ChangeLog, and remember to create an issue to fill those in. As for COPYING, that's traditionally the license file, so we'll just make a copy of doc/license.doc and use that. Now when we run `autoreconf --install` it completes successfully! Ok, let's try running ./configure:

Wow, that worked. Ok, let's try doing a make:

Ah, failure. A quick look at the output shows we're missing some CFLAGS; this might be the source of this compilation error, since one of the compilation flags was -DGROUP_NAMES and that might be what the gname field is for. A quick look at the declaration of struct char_special_data in structs.h confirms this is the case. Ok, so we just need to figure out how to get the CFLAGS declared properly. According to this answer on StackOverflow, it seems we can just add them in the file.


In the process of looking for advice on adding the CFLAGS, I ran across a description of the autoscan tool that will scan your source code and suggest missing macro invocations for your A quick run shows that we're mostly missing detection of the header files we include and the library functions we call, so we'll just add that in too:

AC_CHECK_HEADERS([arpa/inet.h fcntl.h malloc.h netdb.h netinet/in.h
  stdlib.h string.h strings.h sys/socket.h sys/time.h unistd.h])


AC_CHECK_FUNCS([bzero gethostbyaddr gethostbyname gethostname
  getpagesize gettimeofday inet_ntoa isascii memset select socket
  strchr strdup strrchr strstr])! It builds without errors. Now, there's still a few things missing here, like actually using the defined macros in the config.h that the configure script generates; we also haven't gotten the tests running yet, or looked at what "make install" wants to do. So let's get started with the tests. The first thing we're going to want to do is pull the common source code files out into their own variable:

common_sources = comm.c act.comm.c act.move.c act.obj1.c \
 act.obj2.c act.other.c act.wizard.c handler.c \
 db.c interpreter.c utility.c spec_assign.c shop.c limits.c mobact.c \
 fight.c modify.c weather.c spells1.c spells2.c spell_parser.c \
 reception.c constants.c spec_procs.c signals.c board.c magic.c \
 magic2.c skills.c Opinion.c Trap.c magicutils.c multiclass.c hash.c \
 Sound.c Heap.c spec_procs2.c magic3.c security.c spec_procs3.c \
        create.c bsd.c parser.c intrinsics.c
dmserver_SOURCES = $(common_sources) main.c

At this point, while perusing through the automake manual to figure out how to do tests, I discovered there was a better way to define the symbols instead of adding them to CFLAGS in; there's an automake variable for this called AM_CFLAGS, so we just move the flags over to instead. But in the meantime, the next step towards tests would be to correctly find the header files and library for Criterion in the configure script, so that the generated Makefile looks for them in the right place. We can do this by adding the following to

  echo "***WARNING***: unit tests will not be runnable without Criterion library"
  echo "  See"
  echo "***WARNING***: unit tests will not be runnable without Criterion library"
  echo "  See"

After a little more perusing through the autotools manual, it turns out instead of the echo command there's a canonical way to do this using the AC_MSG_WARN macro, as in:

  AC_MSG_WARN(unit tests will not be runnable without Criterion library)

Now, when we then run make, we find the Criterion library, but the final dmserver executable gets linked with -lcriterion, which we don't want, because as you may recall, that library has a main() function in it that is going to try to run test suites, so we don't actually want the default action of AC_CHECK_LIB. Instead, we need to fake it out:

  AC_MSG_WARN(unit tests will not be runnable without Criterion library)

And now we can go ahead and build the unit test program and indicate that's what should run during "make check" by adding the following to src/

# unit tests
check_PROGRAMS = tests
tests_SOURCES = $(common_sources) test.act.wizard.c
tests_LDADD = -lcriterion
TESTS = tests

Sure enough, we can run the tests:

Nice. Ok, now these hardcoded AM_CFLAGS are still bothering me. Really, we ought to be able to opt into and out of them via feature enabling/disabling from configure. My friend Dan would probably, at this point, say "Why?" in incredulity, but this is not an exercise in practicality, per se... The way we do that is to add these flags to, which will cause configure to output them into config.h. We can do that with stanzas like the following:

        [Define as 1 to restrict each level 58+ god to one site or set of
    [turn off site-locking for gods])],

Ok, then we need to just go around and include config.h in all the .c and .h files, and then we can remove the AM_CFLAGS from Cleaner! At this point, the last thing to do is to get make install to work. It turns out the default action for the MUD server itself is the right one, but we also need to collect the data files and install them. This can be done by creating Makefile.ams in various subdirectories. For example, here's the one for the top-level lib/ directory:

Then the last thing to is to make the compiled server look there by default. The configure script takes a --localstatedir argument to customize where those data directories get created; we really want the game to have that path compiled into it as a default path. After much noodling through StackOverflow and the automake manuals, it looks like the best way to do this is a multi-step process in order to get the right amount of variable expansion. First, we have to bring our friend AM_CFLAGS back and pass the Makefile level variable $(localstatedir) in as a symbol definition:

AM_CFLAGS = -DLOCAL_STATE_DIR=\"$(localstatedir)\"

Then, we can add the following to

  [Default location to look for game data files])

...which results in the following showing up in config.h:

/* Default location to look for game data files */

This makes whatever got passed in to the --localstatedir argument of configure, which defaults to /usr/local/var, to show up as a string literal LOCAL_STATE_DIR. In C, two string literals adjacent to one another get concatenated, so this results in DEFAULT_LIBDIR being something like /usr/local/var/sillymud. At this point, we're able to run make install and run /usr/local/bin/dmserver. I think for our next episode, it's time to do a little play testing to see how well things are working and what else needs fixing!

[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.

Saturday, November 14, 2015
VerySillyMUD: Fixing the Rest of the Compiler Warnings

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 used unit tests to document a function's behavior so we could refactor it to fix a security warning. Now we are able to pick up where we left off three episodes ago: fixing compiler warnings to find any gotchas. To get started, let's re-enable the -Werror compiler flag. Now, back to fixing warnings. I'll just highlight the ones here that are novel cases we haven't seen before. First up:

Ok, let's get some context; here's the function where that warning/error appears:

So we see that d is a struct descriptor_data *. A little grepping around shows that's defined in structs.h:

Sure enough, we can see that d->host is a statically-allocated array. Now, this seems like a recipe for a buffer overrun to me, but one thing at a time. We'll maybe find or fix that later by running lint or valgrind. For now, though, I'll create an issue so we don't forget, then we can just fix this to a simpler test and move on.

Well, it turns out that was the most interesting warning/error to highlight. In almost all cases it was pretty clear how to fix the warning, and often the compiler's error message told me exactly how to fix it! Since there were a LOT of changes to get here, we'll spare you the details. At this point, the game starts without most of the warnings we used to see. Hurrah! Now the only question is what to do for the next episode...

[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.

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.