Detecting Problems With -fsanitize

In the past few months I have been working on adding time zone support to MongoDB's Aggregation Framework. This support brings in the timelib library that is also used in PHP and HHVM to do time zone calculations. One of the stages in our workflow before we commit code to master, is to put our patches up onto our continuous integration platform Evergreen, where tests are run against multiple platforms. You expect the usual Ubuntu, RHEL and Windows platforms, but we also run on more esoteric platforms like s390. We also define a few special platforms that run tests in an environment where the code has been compiled in a special mode to test for undefined behaviour in the C language, and memory leaks.

One of the issues that this found (quite quickly) in timelib, was:

src/third_party/timelib-2017.05beta3/parse_tz.c:152:16: runtime error:
  left shift of 128 by 24 places cannot be represented in type 'int'

Which referred to the following code:

buffer[i] = timelib_conv_int(buffer[i]);

timelib_conv_int is a macro defined as:

#define timelib_conv_int(l) ((l & 0x000000ff) << 24) + \
        ((l & 0x0000ff00) << 8) + ((l & 0x00ff0000) >> 8) + \
        ((l & 0xff000000) >> 24)

The sanitiser stumbled over some of the data in the Olson database where we attempted to shift the unsigned integer 128 left by 24 positions into an signed integer, which of course can not represent this value. Thanks to the sanitizer, beta4 has this problem now fixed.

As part of the fix, I investigated how our tests managed to figure out that there was undefined behaviour. It appeared that GCC and clang have a specific flag to enable this debugging tool. It is as simple as adding -fsanitize=undefined to your build flags, which is what timelib in its standalone Makefile now includes.

One of the things that is tricky is that when writing C++, I tend to use many C-isms as that is what I have been working in for so long. And the opposite is true to. C++-isms are sometimes used when dealing with the timelib library which is written in C. One of these issues created a memory leak (fixed through this patch), as a structure was not allocated on the heap, but on the stack. This structure (timelib_time*) sometimes contains an extra string (for the time zone abbreviate) that needs to be freed if set.

This memory leak was also discovered by a sanitizer flag, but this time it was -fsanitize=address. This flag adds code to the compiled binary to test for memory leaks, overflows, etc.—not to dissimilar as to what Valgrind (I wrote about that before) provides. After adding this flag to the default build rules for timelib, it quickly found a few other memory leaks in some test and example files which I then addressed.

So there we have it, two new GCC and Clang flags that I did not know about. I now always compile timelib, as well as my local MongoDB test builds with these two flags. It certainly slows down execution, but that's a cheap price to pay to prevent bugs from making it into production.