This Old Code: Renovating Dusty Old Open Source Code For Fun & Profit

Software is a human construct and like everything we make it eventually falls into disrepair. Houses get mould, vermin infestations, and frightening old wiring. Software projects get equivalents of these, albeit a few decades faster.

This talk is about the experience of doing a major renovation on a software relic: the Cyrus IMAP server. Fastmail (now Opera Software Australia) has been using Cyrus commercially to provide an IMAP interface to an email store for years now, and over the last year has significantly contributed to a resurgent effort to modernise the code and rejuvenate the community. The author works fulltime on this project and has both observed and contributed.

Topics will include: introducing modern software engineering practices like Continuous Integration, making testing happen, attracting a developer community, a practical guide on how to find and fix outdated coding practices (a.k.a. bugs waiting to happen), and an introduction to paleoentomology. All illustrated with real world examples.

If your organisation depends on ancient software in need of revitalising, or if you're just looking for some spare time Open Source work to keep your hand in, come to this talk and get some practical tips and a laugh or two.

Transcript

1. This Old Code Renovating Dusty Old Open Source Code For Fun & Profit This Old Code This Old Code Greg Banks <gnb@fastmail.fm>
2. • Introduction & Background, or Who Is This Guy Anyway? • Modern Software Engineering Practices, or Replacing The Wiring • Attracting A Developer Community, or If You Build It • Finding and Fixing Bad Practices, or Termite Hunting • War Stories, or Stuff I Didn't Expect To Happen • End Matter, or Sisyphus At The Keyboard Overview
3. Introduction & Background
4. Who am I? • Greg Banks • Been doing C & C++ on UNIX for Too Long Now • Last time I spoke at LCA was working on NFS for SGI • Now working on Cyrus for FastMail.FM
5. What's FastMail.FM? • Small Australian web email provider – Targets a niche Power User / Professional audience • Started 2000, consistently profitable since • Bought by Opera Software 2010 • Technology now powers mail.opera.com – Full AJAX design with caching, pre-fetching and optimistic actions – Conversations support across folders – Push updates when new email arrives .fm = Federated States of Micronesia
6. • IMAP / POP3 / NNTP email server • Stores and manages email • Open Source – (c) Carnegie Mellon University – BSD-like licence • Written in C • Runs on UNIX systems • VCS dates go back to Jul 1993 What's Cyrus?
7. ...and how did it get so dusty? • Natural process of bitrot due to advanced age • Originally written before – C89 compilers common – C++ & Java – OO or TDD were widely known – Purify and Valgrind – MIME and Unicode • Email is just a Maze of Twisty Little RFCs – IMAP in particular is quite funky
8. The Email RFC Jungle • Message format: 822→2822→5322, 2045, 2046, 2047, 2231 • IMAP: 1064→1176→1730→2060→3501, 2086 (ACLs) 2087 (QUOTA) 2088 (sane literals) 2193 (mailbox referrals) 2221 (login referrals) 2971 (ID) 4466 (misc) 4469 (CATENATE) 4551 (CONDSTORE), 5032 (WITHIN), 5255 (I18N), 5256 (SORT), 5257 (ANNOTATE), 5464 (METADATA), 5738 (UTF-8), 5804 (ext LIST), 5957 (sorting), 5162 (QRESYNC), 6154 (specialuse) • MUPDATE: 3656 SASL: 2222 LMTP: 2033 • SIEVE: 3028→5228, 5804 • POP3: 1081→1225→1460→1725→1939, 2449 • NNTP: 977→3977, 2980, 4642, 4644
9. FastMail and Cyrus • FastMail use Cyrus to store customers' email – Must be fast and reliable • So we need to – Renovate the code = less bugs, more developers – Have an active community = more eyes on bugs • FastMail's Bron Gondwana started pushing Cyrus modernisation a few years back • I was hired in 2010 to work full-time on Cyrus • This talk represents our collective experience
10. Cyrus Feature Work • Conversations – Like threads but cross-folder, server-side, persistent – Coming in Cyrus 2.6 – Running on beta.fastmail.fm now • Per-message annotations (RFC 5257) • Annotator daemon • Multiple quota resources (RFC 2087) – All coming in Cyrus 2.5
11. Modern Software Engineering Practices
12. Distributed Version Control • Use GIT! • If you're on CVS or SVN, shift now • github.com and sourceforge.com provide free git hosting • Trivial to set up your own hosting • It takes a little getting used to but it's worth it – You'll never have to fiddle with patches again – Learn to use branches effectively – Beware the frumious non-fast-forward push
13. Making Testing Happen • Have a system test suite • Have unit tests too – This often forces you to refactor code for testability • Use a test framework – We use CUnit for unit tests, Test::Unit for system tests • Run your tests often – Run C code under Valgrind • Maintain your tests • Do coverage studies and improve the tests
14. Making Testing Happen (2) • Practice Test Driven Development – For every bug: – Start by writing a failing test case – Then fix the bug – Then show the test case passing • Nag people who... – update the code but not the tests – break the test – don't write regression tests when fixing bugs
15. CUnit Test Framework – Almost Useful • Stable • Widely available • Works in C-only environment – without C++ • BUT! Primitive. Had to add... – Assertions which log failing operands – Automatic test discovery – Useful test selection/filtering – Handle assert, syslog, exit or SEGV in SUT – Test timeout – Parameterised tests – jUnit XML result format
16. Test::Unit Test Framework – Useful • Stable • Widely available • Reasonable xUnit implementation • Does jUnit output format via an add-on • BUT! Imperfect. Had to add... – Better test selection/filtering – Print stack traces in failure messages
17. Cassandane • New system test suite for Cyrus • Written in Perl, uses Test::Unit • Creates and manages temporary Cyrus instances – Allows precise control of server configuration – Each test gets a known initial state • Utility code to create messages in arbitrary ways – Allows precise control of folder contents • Uses Mail::IMAPTalk et al to talk to servers • Optionally runs all Cyrus code under Valgrind
18. Bug Tracking System • Have a Bug Tracking System • Bugzilla or JIRA commonly used today – They all suck in various ways, deal with it • Make it publicly accessible • Have a designated Release Manager person – Who actively maintains and updates the BTS – Jeroen van Meeuwen of Kolab Systems
19. Coding Standards • Have common standards – Unreadable code is buggy code – Shared code is not a place to exercise freedom of expression • Document them, explicitly, with examples • Stick to them! • Write an .indent.pro to enforce them • Reformat to achieve them – But never mix reformatting with functional changes
20. Continuous Integration • Build and test for every commit • I like Jenkins (formerly Hudson) • Well supported and actively maintained • More plug-ins than you can imagine • Good web interface • Polls GIT (or use a trigger) • Integrates with jUnit test results & coverage – Displays useful graphical reports for each build • Have been running hudson.cyrusimap.org for 6 months – Now trying to integrate with Cassandane system tests
21. Attracting A Developer Community
22. Good OSS Project Management • Have a website yourproject.org • Git repo • Mailing lists, IRC, Bug Tracking System • Separate stable & release branches • Regular releases • Downloadable release tarballs
23. Play Well With Others • Have a designated maintainer who is active • Be responsive to queries and patches • Always be polite • Make an effort to merge people's patches • Be grateful for any contributions
24. The Cyrus Community • Core Contributors – Alexey Melnikov (Isode), Bron Gondwana (FastMail), Dave McMurtrie (CMU), Greg Banks (FastMail), Jeroen van Meeuwen (Kolab), Ken Murchison (CMU) • Current and Past Contributors – Wes Craig, Benn Oshrin, Matt Selsky, Jeffrey T. Eaton, Yoni Afek, Leena Heino, Dan White, Kristof Vansant, Øyvind Kolbu, Ondřej Surý, Ralf Haferkamp, Max Matveev, Gildas Desnos, Guilherme Maciel Ferreira, Richard Bos, Henrique de Moraes Holschuh, Chaskiel Grundman, Derrick Brashear, Rob Siemborski, Larry Greenfield, Walter Wong, Jen Smith, Mark Tyrrell, Tim Martin, Tim Showalter, Dan Root, Sam Weiler, John Gardiner Meyers, Chris Newman, Laurie D. Mann, Simon Matter
25. Finding & Fixing Bad Practices
26. GCC Warning Flags • GCC has lots of very useful warning flags • Which are all off by default • Choose one, enable it, fix the build, rinse & repeat • My favourites – -Wall despite the name, just basic warnings – -Wextra unused variables, enums not handled in switch, signedness issues, type punned pointers – -Wstrict-prototypes antique K&Risms – -Wpointer-arith abuse of void* – -Wwrite-strings abuse of string literals • Some warnings only appear when using -On
27. Coverity Scanner • Coverity make a commercial product which uses source code analysis to find problems in C source • The US Government pays them to provide a free hosted service for Open Source projects • Go register at scan.coverity.com and follow it up • There will be lots of false positives to wade through – Over 1000 reports for Cyrus • But worth it because it finds stuff you never thought of. – CVE-2011-3208
28. The Hard Way • Use grep to search for warning signs – grep -w realloc $(git ls-files) • Write custom scripts • Inspection – Read The Code
29. Danger Signs • Danger – A bug waiting to happen – Maybe even a remotely exploitable security hole – Hunt them down and analyse or replace them! • Ugly – Old fashioned, unnecessary or confusing code – Fixing these helps readability in the long term • Better – Code that has worked fine for years – But a better way to solve the problem now exists
30. Danger: Use of realloc • It's great that the code doesn't assume fixed sizes, but... • The pointer arithmetic is error prone • Accounting to amortise realloc calls is tedious • Handling allocation failure is hardly ever done right • Testing all the cases can be hard • You want to be using a separate and well-tested library – An expanding string module – An expanding string array module – An expanding pointer array module – An expanding array module • The same goes for many uses of memmove, memcpy
31. Danger: Use of realloc (2) - static char *body = NULL; - static int bodylen = 0; - int off = 0; + static struct buf body = BUF_INITIALIZER; - if (bodylen == 0) { - bodylen += BODYINC; - body = (char *) realloc(body, bodylen * sizeof(char)); - } + buf_reset(&body); - body[off++] = c; - if (off >= bodylen - 3) { - bodylen += BODYINC; - body = (char *) realloc(body, bodylen); - } + buf_putc(&body, c);
32. Danger: Use of sprintf, strcpy • Non-boundary checked string functions – Buffer overruns waiting to happen – These functions should never have existed • Use the equivalent boundary checked versions – Watch out, strncpy and snprintf overflow unobvious – Better: use an expanding string library char buf[32]; -sprintf(buf, “line %d: %s”, lineno, msg); +snprintf(buf, sizeof(buf), “line %d: %s”, lineno, msg); +buf[sizeof(buf)-1] = '\0';
33. Danger: Use of malloc, strdup • Direct use of memory allocation functions • Out Of Memory handling code is never run, never tested – Drags down coverage & expectations of coverage • Use a wrapper which handles OOM by logging & exiting – e.g. xmalloc like malloc but never returns NULL - b->buf = malloc(b->buf_size); - if (b->buf == NULL) { - squat_set_last_error(SQUAT_ERR_SYSERR); - return SQUAT_ERR; - } + b->buf = xmalloc(b->buf_size);
34. Danger: open coded data structures • Everyone wants to write their own list or hash table... • Use the perfectly good ones in glib or on CCAN - clinit = cl = hashheader(head); - while (m->cache[cl] != NULL) { - if (!strcmp(head, m->cache[cl]->name)) { - *body = (const char **) m->cache[cl]->contents.data; - break; - } - cl++; /* try next hash bin */ - cl %= HEADERCACHESIZE; - if (cl == clinit) break; /* gone all the way around */ - } + *body = (const char **)hash_lookup(head, &m->cache);
35. Danger: open coded crypto • Open-coded common algorithms – MD5 – SHA1 – CRC32 – Base64 – Bin to hex • Seriously? Use libraries!
36. Danger: error muddling • Mixing conventions for returning errors – Return -1, <errno.h> error code in errno – Return -ve <errno.h> code – Return +ve <errno.h> code – Return -ve application error code • Impedance mismatch means error handling is buggy • It doesn't matter, choose one and stick to it – Code is easier to read and understand • Use a single combined error code space – Not different error code spaces per module – Use the com_err library
37. Danger: printf-like functions • Functions which take printf-like format & args – Wrappers around vfprintf or vsnprintf – Often used for logging • This defeats normal compiler type-checking! • GCC has a -Wformat warning option • But you need to enable it on function declarations: -int prot_printf(struct protstream *, const char *, ...); +int prot_printf(struct protstream *, const char *, ...) + __attribute__ ((format (printf, 2, 3)));
38. Ugly: K&R functions • Old K&R style function declarations & definitions • Surprisingly, still accepted by compilers • Ugly and clumsy • Less compiler type checking -extern void cmdtime_starttimer(); +extern void cmdtime_starttimer(void); -void -map_free(base, len) -const char **base; -unsigned long *len; +void map_free(const char **base, unsigned long *len)
39. Ugly: K&R <varargs.h> • It's hard to believe it used to be this clumsy -#include <varargs.h> +#include <stdarg.h> -int foo(va_alist) -va_dcl; +int foo(struct bar *b, ...) { - struct bar *b; - va_list args; - va_start(args); - b = va_arg(args, struct bar *);
40. Ugly: relics of C89 transition • There was a time when code needed to compile in both K&R and ANSI C compilers • This was solved with #ifdef __STDC__ • That time was 20 years ago – don't ever use __STDC__ -#ifdef __STDC__ void imclient_addcallback(struct imclient *imclient, ...) -#else -void imclient_addcallback(va_alist) -va_dcl -#endif
41. Ugly: const uncorrectness • Const provides useful compiler checking • Also serves as a hint to a human reading the code • Useful on pointers to structs and strings -void *hash_del(char *key, hash_table *table) +void *hash_del(const char *key, hash_table *table)
42. Ugly: free(NULL) • In the Bad Old Days, free(NULL) crashed • Now it's defined to harmlessly do nothing • So checking is unnecessary and old-fashioned -if (x) free(x); +free(x);
43. Ugly: think global, link local • Unnecessarily global functions and variables • Pollute the global namespace...untidy. • Can lead to ugly surprises when mixing libraries -char *parse_host(char *listen) +static char *parse_host(char *listen)
44. Ugly: BDSMBLS • Bad symbol names make code unreadable – Unreadable code is harder to work with, hence buggier • Rename symbols with: – No vowels – No word separators – Extreme abbreviations – No obvious semantic connection – “Similarity” with libc names is not an excuse -void cyrus_ctime(time_t date, char *datebuf) +int time_to_rfc3501(time_t date, char *buf, size_t len)
45. Ugly: whitespace orthodoxy • This is a Linux kernel dogma – Designed to make it easier to read unified diffs in text – Because kernel folk have something against xxdiff or meld • This dogma was encoded in git warnings – So now we all have to be picky about whitespace • Use vim macros to highlight bad whitespace in red – At least 2 such vim plugins on github – I use Keith Owens', modified
46. Ugly: duplicate code • Many cooks, each implementing their own. • You don't need two! This is why we have libraries. -struct ibuf { - char *start, *end, *last; -}; typedef struct { int len; /* Data immediately following... */ } mystring_t; struct buf { char *s; unsigned len, alloc; int flags; };
47. Ugly: too much argufying • Any function with more than 5 arguments has Been Designed Wrong int mboxlist_createmailbox_full(const char *name, int mbtype, const char *partition, int isadmin, const char *userid, struct auth_state *auth_state, int options, unsigned uidvalidity, const char *copyacl, const char *uniqueid, int localonly, int forceuser, int dbonly, struct mailbox **mboxptr, struct dlist *extargs) {
48. Ugly: magic numbers • I shouldn't have to explain this • It was bad practice 30 years ago - for (i = 0; i < 10; i++) + for (i = 0; i < CACHE_NUM; i++)
49. Better: single error path • Taken from the Linux kernel style • Do all the cleanups for error handling in a single block at the end of each function with the label out: • All the error handling becomes goto out; • Reduces the size of the code • Makes error handling less tedious and inaccurate • Probably the only un-Harmful use of goto
50. Single error path example int inpipe[2] = { -1, -1 }; int r = IMAP_SYS_ERROR; ... pid = fork(); if (pid < 0) { syslog(LOG_ERR, "cannot fork: %m"); goto out; } ... r = 0; out: if (inpipe[PIPE_READ] >= 0) close(inpipe[PIPE_READ]); if (inpipe[PIPE_WRITE] >= 0) close(inpipe[PIPE_WRITE]); return r;
51. Better: strconcat • Lots of tiresome and potentially hazardous string futzing can be replaced with a function like char *strconcat(const char *s1, ...); char *scrname; - scrname=malloc(strlen(name)+10); - strcpy(scrname, name); - strcat(scrname, ".script"); + scrname = strconcat(name, ".script", (char *)NULL);
52. Better: xstrndup • Sometimes you want a new copy of a piece of memory • Using malloc+memcpy/strcpy is tedious • Accounting for trailing NUL is error prone char *xstrndup(const char* str, unsigned len); void *xmemdup(const void *ptr, unsigned size); - len = hdrend - hdr; - *hdrp = malloc(len + 1); - strlcpy(*hdrp, hdr, len + 1); + *hdrp = xstrndup(hdr, (hdrend - hdr));
53. Better: strcmpsafe • Standard strcmp will SEGV if given a NULL pointer. • Wrap it in a function which treats NULL arguments like “”. int strcmpsafe(const char *a, const char *b); - r = strcmp((a->domain == NULL ? "" : a->domain), - (b->domain == NULL ? "" : b->domain)); + r = strcmpsafe(a->domain, b->domain);
54. Better: strarray_t • Manipulating an expanding array of strings is a very common job – Building argv[] for execv – Lists of tokens – Lists of filenames • Using realloc to manage memory is tedious • Pointer arithmetic for insert etc is error prone • Perl-like join and split operations are surprisingly useful • Passing separate args (const char**, int n) is tedious • Once you have this, you find yourself using it everywhere
55. Using strarray_t -static char **tokenize(char *p) -{ - char **tokens = NULL; - int i = 0; - - if (!p || !*p) return NULL; /* sanity check */ - while (*p) { - while (*p && Uisspace(*p)) p++; /* skip whitespace */ - - if (!(i % 10)) - tokens = xrealloc(tokens, (i+10) * sizeof(char *)); - - /* got a token */ - tokens[i++] = p; - while (*p && !Uisspace(*p)) p++;
56. Using strarray_t (2) - /* p is whitespace or end of cmd */ - if (*p) *p++ = '\0'; - } - /* add a NULL on the end */ - if (!(i % 10)) - tokens = xrealloc(tokens, (i+1) * sizeof(char *)); - if (!tokens) return NULL; - tokens[i] = NULL; - - return tokens; -} - Services[i].exec = tokenize(cmd); + Services[i].exec = strarray_split(cmd, NULL);
57. Better: tok_t • Using strtok is error prone – hidden state • Using strtok_r properly is tedious • Ditto strsep if you want to see empty tokens • So wrap all that in a simple object char *line, *p; tok_t tok = TOK_INITIALIZER(line, NULL, 0); while ((p = tok_next(&tok)) printf(“%s\n”, p); tok_fini(&tok);
58. Better: undangling APIs • Typical C “object” API does struct mailbox *mailbox_open(const char *name); void mailbox_close(struct mailbox *); • No error code is returned when open fails • A mailbox* passed to mailbox_close is now dangling – Which can lead to all sorts of awful subtle failure modes – For safety needs to be NULLed out after call
59. Better: undangling APIs (2) • Or you could design the API right! int mailbox_open(const char *name, struct mailbox **); • Error code is returned if open fails • After return, pointer is always NULL or valid void mailbox_close(struct mailbox **); • Handles pointer being either NULL or valid – Safe to call regardless of whether open succeeded • After return, pointer is always NULL, no dangling
60. Better: gperf • Long lists of if (!strcmp(x, "y")) are slow and tedious – e.g. parsing protocol commands • The GNU gperf utility takes a set of strings and emits a perfect hash function in C – Calculates a collision-free hash in O(strlen(s))
61. Better: make -j • N-way MP machines are common these days • Every Makefile should support make -j (parallel make) • But this not always obvious • Some things work by accident in single-threaded make – Insufficient dependencies in the Makefile – But build order happens to build things right – UNTIL you run it in parallel • More subtle problem of a command building two outputs – e.g. yacc and rpgen – This is why those programs have options to build only a single output at a time
62. War Stories
63. Paleoentomology • (n) the study of ancient bugs – Not to assign blame – To discover how it happened & prevent it happening again – To discover in which releases it escaped • Using git blame -s – Easy when the bug involves adding or changing code – Need to bisect the history when the bug involves code that went missing
64. Refactoring and Sharp Edges • Refactoring sometimes breaks stuff • Even obviously correct small changes • You need tests – And humility ­ i­>sval = xmalloc(len+1); ­ memcpy(i­>sval, val, len); ­ i­>sval[len] = '\0'; /* make it string safe too */ + i­>sval = xstrndup(val, len); • In this case the buffer may legitimately contain \0s – The comment is...misleading • xstrndup fails to copy past them...breaking email fltering
65. No Plan Survives Contact • Conversations code join messages into conversations • It scans for message-ids in header fields – Message-ID: – In-Reply-To: – References: • In RFC 2822, In-Reply-To: is strictly a sequence of message-ids • The older RFC 822 was vaguer • Older versions of the NMH mailer produced things like In-Reply-To: Message from Fred Bloggs <fred@bloggs.com> of "Sun, 01 Nov 2009 04:11:30 BST." <20091101031130.GA5708@foo.bar.org> • The address looks like a message-id to the scanning code!
66. No Plan Survives Contact (2) • Leads to massive confusion – Spurious (expensive) conversation joins – Humungous conversations (>2000 messages) • You can't even blame the mailer authors – Code was valid under RFC 822 when written – Invalid now under RFC 2822 – Fixed in current versions – But older versions are still out there • Solution was to ignore In-Reply-To: if it didn't start with '<'
67. The Infamous May 13 Incident • Cyrus has a config file annotations_definitions • It's normally non-existant • I committed a 1-line file to the FastMail config • But not the version that I had tested and fixed in a VM • So, the file committed had one comma (,) too many • This “trivial, obvious change” was later rolled out to all servers
68. May 13 Incident: Detonation • On start, Cyrus' imapd process detects this syntax error • And immediately calls fatal which calls exit • Cyrus' master process detects imapd dying • And immediately restarts imapd • Looping forever – Chewing up CPU – Spewing to logs – Denying any IMAP service
69. May 13 Incident: Cascade • EXCEPT! In FastMail, fatal calls abort • So each iteration of the loop dumps a core • In the «meta» partition where the databases live • The smaller servers filled up in a few minutes • It turns out, Cyrus' database code is buggy at ENOSPC • So many mailboxes.db were corrupted
70. May 13 Incident: Fallout • DBs had to be recovered from replicas, and backups – Several hours • No emails or folders were lost – Some \Seen flags were – No mail service during outage • Messages and quotas needed checking – About two days • Lesson: cascading failures suck!!
71. Advertising Weakness • Putting out a security release with a CVE number – Attracts security researchers who look at your code – And they promptly find new security problems! • Release 2.4.11 dated 9 Sep 2011 – Patched CVE-2011-3208 – Remote exploit in the NNTP daemon • Release 2.412 dated 5 Oct 2011 – Patched Secunia SA46093 – Stefan Cornelius discovered another bug in the same code
72. End Matter
73. Remaining Work • A new message API • Use list and array libraries instead of open-coding • Central and controllable logging instead of a mix of syslog & printf • Express mboxnames as a struct not a string • Convert to automake & libtool shared libraries • Be consistent between abort, fatal, and assert • Add lots more Cassandane tests • Finish converting old unused standalone test code to CUnit tests • Finish going through the Coverity scans • Refactor to use the callback+rock pattern much less
74. Acknowledgements • Opera Software for paying my salary • Andrew Wansink for leading the way with CUnit • Keith Owens for vim whitespace macros • CMU for starting Cyrus all those years ago • CMU for providing the cyrusimap.org infrastructure today
75. References • www.cyrusimap.org • www.fastmail.fm • mail.opera.com • CUnit http://cunit.sourceforge.net/ • Test::Unit http://search.cpan.org/~mcast/Test-Unit-0.25_1325/ • Cassandane http://git.cyrusimap.org/cassandane • Jenkins jenkins-ci.org • Valgrind valgrind.org • Coverity scan.coverity.com • RFCs http://tools.ietf.org/html/ • gperf http://www.gnu.org/software/gperf/manual/gperf.html • CCAN http://ccodearchive.net/ • GLib http://developer.gnome.org/glib/2.30/ • GCC http://gcc.gnu.org/onlinedocs/gcc-4.6.2/gcc/Warning-Options.html • Mail::IMAPTalk https://github.com/robmueller/mail-imaptalk • GitHub github.com • SourceForge sourceforge.com
76. www.opera.com Oh, we're HIRING! http://www.opera.com/company/jobs