CodeSOD: A Maskerade

Josh was writing some code to interact with an image sensor. Fortunately" for Josh, a co-worker had already written a nice large pile of utility methods in C to make this easy".

So, when Josh wanted to know if the sensor was oriented in landscape or portrait (or horizontal/vertical), there was a handy method to retrieve that information:

// gets the sensor orientation// 0 = horizontal, 1 = verticaluint8_t get_sensor_orient(void);

Josh tried that out, and it correctly reported horizontal. Then, he switched the sensor into vertical, and it incorrectly reported horizontal. In fact, no matter what he did, get_sensor_orient returned 0. After trying to diagnose problems with the sensor, with the connection to the sensor, and so on, Josh finally decided to take a look at the code.

#define BYTES_TO_WORD(lo, hi) (((uint16_t)hi << 8) + (uint16_t)lo)#define SENSOR_ADDR 0x48 #define SENSOR_SETTINGS_REG 0x24#define SENSOR_ORIENT_MASK 0x0002// gets the sensor orientation // 0 = horizontal, 1 = vertical uint8_t get_sensor_orient(void) { uint8_t buf; read_sensor_reg(SENSOR_ADDR, SENSOR_SETTINGS_REG, &buf, 1); uint16_t tmp = BYTES_TO_WORD(0, buf) & SENSOR_ORIENT_MASK; return tmp & 0x0004; }

This starts reasonable. We create byte called buf and pass a reference to that byte to read_sensor_reg. Under the hood, that does some magic and talks to the image sensor and returns a byte that is a bitmask of settings on the sensor.

Now, at that point, assuming the the SENSOR_ORIENT_MASK value is correct, we should just return (buf & SENSOR_ORIENT_MASK) != 0. They could have done that, and been done. Or one of many variations on that basic concept which would let them return either a 0 or a 1.

But they can't just do that. What comes next isn't a simple matter of misusing bitwise operations, but a complete breakdown of thinking: they convert the byte into a word. They have a handy macro defined for that, which does some bitwise operations to combine two bytes.

Let's assume the sensor settings mask is simply b00000010. We bitshift that to make b0000001000000000, and then add b00000000 to it. Then we and it with SENSOR_ORIENT_MASK, which would be b0000000000000010, which of course isn't aligned with the layout of the word, so that returns zero.

There's no reason to expand the single byte into two. That BYTES_TO_WORD macro might have other uses in the program, but certainly not here. Even if it is used elsewhere in the program, I wonder if they're aware of the parameter order; it's unusual (to me, anyway) to accept the lower order bits as the first parameter, and I suspect that's part of what tripped this programmer up. Once they decided to expand the word, they assumed the macro would expand it in the opposite order, in which case their bitwise operation would have worked.

Of course, even if they had correctly extracted the correct bit, the last line of this method completely undoes all of that anyway: tmp & 0x0004 can't possibly return a non-zero value after you've done a buf & 0x0002, as b00000100 and b00000010 have no bits in common.

As written, you could just replace this method with return 0 and it'd do the same thing, but more efficiently. Zero" also happens to be how much faith I have in the developer who originally wrote this.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!