embedded software boot camp

This Code Stinks! The Worst Embedded Code Ever

Thursday, November 5th, 2009 by Michael Barr

At the Embedded Systems Conference Boston in September, I gave a popular ESC Theater talk titled “This Code Stinks! The Worst Embedded Code Ever” that used lousy code from real products as a teaching tool. The example code was gathered by a number of engineers from a broad swath of companies over several years. (Minor details, including variable names and function names, were changed as needed to hide the specifics of applications, companies, or programmers.)

Here’s just one example of the bad code in that presentation:

y = (x + 305) / 146097 * 400 + (x + 305) % 146097 / 36524 * 100 + (x + 305) % 146097 % 36524 / 1461 * 4 + (x + 305) % 146097 % 36524 % 1461 / 365;

I don’t know if the above snippet contains any bugs, as most of the other examples were found to. And that’s a problem. Where are we supposed to begin an analysis of the above? What is this code supposed to do when it works? What range of input values is appropriate to test? What are the correct output values for a given input? Is this code responsible for handling out of range inputs gracefully? In the original listing, there were no comments on or around this line to help.

I eventually learned that this code computes the year, with accounting for extra days in leap years, given the number of days since a known reference date (e.g., January 1, 1970). But I note that we still don’t know if it works in all cases; despite it being present in an FDA-regulated medical device. I note too that the Microsoft Zune Bug was buried in a much better formatted snippet of code that performed a very similar calculation.

Here’s another example, this time in C++, with the bug finding left as an exercise for the reader:

bool Probe::getParam(uint32_t param_id, int32_t idx)
{
int32_t val = 0;
int32_t ret = 0;

ret = m_pParam->readParam(param_id, idx, &val);

if (!ret)
{
logMsg(“attempt to read parameter failed\n”);
exit(1);
}
else …

Hint: This code was embedded in a piece of factory automation equipment.

I’ve placed the full set of slides online at http://bit.ly/badcode.

Tags: , , , , ,

3 Responses to “This Code Stinks! The Worst Embedded Code Ever”

  1. Duncan says:

    Wow… let's see:- m_pParam is not checked to see if it is a valid pointer- if (!ret) for a 32-bit int is a loosely defined check, as the boolean state of 1 is not necessarily 0 (especially for signed integers)- the log fails to mention what parameter failed- then it forces an exit of the program, probably not cleaning everything up nicely in the process instead of returning with a failed condition to be handled gracefullyDid I miss anything?

  2. PParkBoltFan says:

    If this is an embedded program, you should never call exit. Exit to what? I suppose it would be possible to setup a reset/exit vector or a watchdog timer that would cause the whole thing to reset after the exit call. Both bad form if by design.

  3. Ulrich says:

    The upper snippet (formula) — to be concrete: the values 365 and 36524 — makes me thinking that it calculates the weekday of a date x given in julian format… But I don't dare to try it… no time 😉

Leave a Reply to Duncan