DrGrizz's blog

Integers footgun

Recently, a colleague of mine proposed the following change in our code base

Change:

    memcpy(network_buffer,
           original_message,
           MIN(original_message_size, OUT_BUFFER_MAX_SIZE));

To

    memcpy(network_buffer,
           &original_message[2],
           MIN(original_message_size - 2, OUT_BUFFER_MAX_SIZE));

The original intend was to skip the first two bytes (if any) because we didn't need them.

To have the full picture,

• network_buffer is defined as char[OUT_BUFFER_MAX_SIZE], 0-initialized.
• original_message is char[2*OUT_BUFFER_MAX_SIZE], , 0-initialized as well.
• By design, original_message is either empty (original_message_size is then 0) or not , then has original_message_size 18 characters.
• OUT_BUFFER_MAX_SIZE is 16
• original_message_size is uint8_t

Let's do the review:

• If cpp original_message is not empty, then original_message_size - 2 is equal to OUT_BUFFER_MAX_SIZE and we we fill network_buffer, skipping its first 2 characters.
• If original_message is empty, then original_message_size - 2 underflows as original_message_size is unsigned and unsigned arithmetic underflow is defined, such that MIN(original_message_size - 2, OUT_BUFFER_MAX_SIZE) is 16, and we fill 16 0 into network_buffer in an array already filled with 0.

LGTM, right ?

And of course, it's not. Because original_message_size is a uint8_t, integer promotion kicks in. Integer types smaller than int are promoted when an operation is performed on them, either in a int if int is big enough to cover all values from the original type, or unsigned int otherwise. You can see at work on C++ insight

#include <cstdio>
#include <cstdint>

int main()
{
   uint8_t x = 0;
   auto y = x - 2;

   unsigned int u = 0;
   auto v = u - 2;
}

So in our case, original_message_size is promoted to an int, such that is original_message_size - 2 is -2 when original_message is empty. And from there, memory havoc follows, likely resulting in a segfault.