A Byte Too Far

A short cautionary tale from today. I’ve been modifying some code and one of the changes I made was to use a type of Lol as a key in a map-like structure (key-pair container, uses < operator for comparisons). Structure itself looked like:

1struct Lol
2{
3    byte tab[16];
4    short cat;
5    bool foo;
6};

…and here’s the operator<

1bool Lol::operator<(const Lol& other) const
2{
3    return(memcmp(this, &other, sizeof(other)) < 0);
4}

The problem was - it seemed like sometimes, in seemingly random cases, we’d try to insert an instance of Lol to a container even though exactly the same element was already there. In other words (pseudo code):

1container.insert(std::make_pair(lol, cat));
2lol2 = lol;
3auto it = container.find(lol2);
4// In some cases it == container.end()!

As you’ve already guessed (or perhaps spotted it immediately) - the problem was caused by operator<. The “original” version of the Lol type didn’t have ‘foo’ member, it’s been added quite recently (*cough*by mecough). Can you guess why did it break?

Answer is simple - we compare sizeof(Lol) bytes. Without ‘foo’, it’s 18 bytes (16 + 2), with ‘foo’ - 20 (16 + 2 + 1 + 1 byte worth of padding). The problem is, we only care about 19 bytes, the last one contains random data. In some cases it matched, in others our operator< would return true even if compared objects were exactly the same. I can see what the intent was, author tried to avoid comparing contents of ‘tab’ array by hand and it actually worked fine for a few years, but it’s a ticking bomb. Most people are aware of potential issues with copying class contents using memcpy+sizeof(class) (nuking the vtable for example), but memcmp+sizeof can be just as dangerous.