Binary reader and writer

Question

I recently implemented a binary reader/writer for my multiplayer game.

Now reading bytes looks like this:

static int32_t readInt32(std::vector<uint8_t>& msg, int *off)
{
    if ((*off) > msg.size()) return 0;
    int32_t result; int size = sizeof (result);
    memcpy(&result, msg.data() + (*off), size); (*off) += size; 
    return result;
}

And writing bytes looks like this:

static void writeInt32(std::vector<uint8_t>* msg, int32_t value)
{
    uint8_t const * array = reinterpret_cast<uint8_t const *>(&value);

    for (std::size_t i = 0; i != sizeof(value); ++i)
    {
        (*msg).push_back(array[i]);
    }
}

How can I improve this?

Answer 1

I see a number of things that could help you improve your program.

Fix your formatting

Crowding multiple statements on a single line makes your program harder to read and understand. So instead of this:

if ((*off) > msg.size()) return 0;
int32_t result; int size = sizeof (result);

Write this:

if ((*off) > msg.size()) {
    return 0;
}
int32_t result; 
int size = sizeof (result);

Use the required #includes

The code uses std::vector which means that it should #include <vector>. It was not difficult to infer, but it helps reviewers if the code is complete. For this code, it appears that you need these:

#include <vector>
#include <cstring>
#include <cstdint>

Use C++-style includes

Instead of including string.h you should instead use #include <cstring>. The difference is in namespaces as you can read about in this question. I infer that the code used that because the call to memcpy was not std::memcpy.

Prefer passing a reference to a raw pointer

The writeInt32() function takes a pointer to a vector, but what if the pointer is nullptr? Clearly the function will fail, and probably crash the program if that happens. Better, then, would be to pass a reference instead, which says that it really MUST be a std::vector<std::uint8_t> and nothing else.

Avoid portability problems

The endianness of computers is not fixed by the C++ language standard, so your memory copy or memory casting is not going to work the same way on all machines. This portability problem can be solved by doing one of two things: either rely only on guarantees that are actually made by the standard or use conversion routines to put everything in the same order. This often comes up in network communications, so some platforms have htonl() and friends to convert from "host order" which might be big-endian or little-endian, to network order which is always big-endian. Note, however, that this has its own portability issues because those functions are not part of the C++ standard. So the other way to do it is to do it yourself.

static void writeInt32(std::vector<std::uint8_t>& msg, std::int32_t value)
{
    for (auto i = sizeof(value); i; --i, value >>= 8) {
        msg.push_back(value & 0xff);
    }
}

This is shorter and also portable. In this version, i is declared to be auto but declaring it to be of type std::size_t, as you had it, is another way to do it and even works if you're not using a C++11 compiler.

Be careful with signed and unsigned

In the readInt32 routine, the code compares an int to a size_t, but size_t is unsigned and int is signed. In this case, your off variable should probably be declared as being of std::size_t * type.

Don't fail silently

In the readInt32 function, a returned value of 0 might mean that a value of zero was actually read or it might mean that the offset pointer was off the end of the message. Unfortunately with the current code there's no way to differentiate these cases. I'd suggest throwing an exception instead.

Be careful with memcpy

The code currently catches the case in which the offset is beyond the end of the vector, but what if it's pointing instead to the last byte of the vector? In this case, memcpy will copy that last byte and whatever happens to be in memory beyond that. This is not likely to be a useful value. In this case, I'd recommend not using memcpy at all and rewriting the code to be portable as mentioned above.

Reconsider the interface

Right now, the code consists of two functions which deal with writing/reading a std::int32_t value to/from a message. If your code frequently does things with such messages, it might be better to create a Message class and have member functions that read and write particular types of values, such as your std::int32_t values used here. I'd also recommend that raw pointers such as off not be part of the interface at all.

Answer 2

When you compare (*off) > msg.size(), you should get a compiler warning about comparison between signed and unsigned. Enable more and more compiler warnings until you get it. Then change int *off to std::size_t *off.

The comparison is wrong. It must be (*off) > msg.size() || *off + size > msg.size() to properly check against buffer overflows.

When you feel confident enough, you can omit the parentheses around *off. They are not necessary.

In the writeInt32 function, you can write msg->push_back, which is more idiomatic.

And now to the biggest point of all. When you run this code on your amd64 server and on a SPARC or PowerPC client, this binary reading will not work at all, because some computers are big endian while others are little endian. To solve this problem, don't write your own code for this task, just use an existing serialization library like protobuf.