[C++] Adding a string or char array to a byte vector

Question

I'm currently working on a class to create and read out packets send through the network, so far I have it working with 16bit and 8bit integers (Well unsigned but still).

Now the problem is I've tried numerous ways of copying it over but somehow the _buffer got mangled, it segfaulted, or the result was wrong.

I'd appreciate if someone could show me a working example.

My current code can be seen below.

Thanks, Xeross

Main

#include <iostream>
#include <stdio.h>
#include "Packet.h"

using namespace std;

int main(int argc, char** argv)
{
    cout << "#################################" << endl;
    cout << "#       Internal Use Only       #" << endl;
    cout << "#     Codename PACKETSTORM      #" << endl;
    cout << "#################################" << endl;
    cout << endl;

    Packet packet = Packet();
    packet.SetOpcode(0x1f4d);

    cout << "Current opcode is: " << packet.GetOpcode() << endl << endl;

    packet.add(uint8_t(5))
          .add(uint16_t(4000))
          .add(uint8_t(5));

    for(uint8_t i=0; i<10;i++)
        printf("Byte %u = %x\n", i, packet._buffer[i]);

    printf("\nReading them out: \n1 = %u\n2 = %u\n3 = %u\n4 = %s",
        packet.readUint8(),
        packet.readUint16(),
        packet.readUint8());

    return 0;
}

Packet.h

#ifndef _PACKET_H_
#define _PACKET_H_

#include <iostream>
#include <vector>

#include <stdio.h>
#include <stdint.h>
#include <string.h>

using namespace std;

class Packet
{
    public:
        Packet() : m_opcode(0), _buffer(0), _wpos(0), _rpos(0) {}
        Packet(uint16_t opcode) : m_opcode(opcode), _buffer(0), _wpos(0), _rpos(0) {}

        uint16_t GetOpcode() { return m_opcode; }
        void SetOpcode(uint16_t opcode) { m_opcode = opcode; }

        Packet& add(uint8_t value)
        {
            if(_buffer.size() < _wpos + 1)
                _buffer.resize(_wpos + 1);

            memcpy(&_buffer[_wpos], &value, 1);
            _wpos += 1;

            return *this;
        }
        Packet& add(uint16_t value)
        {
            if(_buffer.size() < _wpos + 2)
                _buffer.resize(_wpos + 2);

            memcpy(&_buffer[_wpos], &value, 2);
            _wpos += 2;

            return *this;
        }

        uint8_t readUint8()
        {
            uint8_t result = _buffer[_rpos];
            _rpos += sizeof(uint8_t);
            return result;
        }
        uint16_t readUint16()
        {
            uint16_t result;
            memcpy(&result, &_buffer[_rpos], sizeof(uint16_t));

            _rpos += sizeof(uint16_t);
            return result;

        }

        uint16_t m_opcode;
        std::vector<uint8_t> _buffer;
    protected:

        size_t _wpos; // Write position
        size_t _rpos; // Read position
};

#endif // _PACKET_H_

Answer 1

Since you're using an std::vector for your buffer, you may as well let it keep track of the write position itself and avoid having to keep manually resizing it. You can also avoid writing multiple overloads of the add function by using a function template:

template <class T>
Packet& add(T value) {
    std::copy((uint8_t*) &value, ((uint8_t*) &value) + sizeof(T), std::back_inserter(_buffer));
    return *this;
}

Now you can write any POD type to your buffer.

implicitly:

int i = 5;
o.write(i);

or explictly:

o.write<int>(5);

To read from the buffer, you will need to keep track of a read position:

template <class T>
T read() {
    T result;
    uint8_t *p = &_buffer[_rpos];
    std::copy(p, p + sizeof(T), (uint8_t*) &result);
    _rpos += sizeof(T);
    return result;
}

You will need to explicitly pass a type parameter to read. i.e.

int i = o.read<int>();

Caveat: I have used this pattern often, but since I am typing this off the top of my head, there may be a few errors in the code.

Edit: I just noticed that you want to be able to add strings or other non-POD types to your buffer. You can do that via template specialization:

template <>
Packet& add(std::string s) {
    add(string.length());
    for (size_t i = 0; i < string.length(); ++i)
        add(string[i]);
    return *this;
}

This tells the compiler: if add is called with a string type, use this function instead of the generic add() function.

and to read a string:

template <>
std::string read<>() {
    size_t len = read<size_t>();
    std::string s;
    while (len--)
        s += read<char>();
    return s;
}

Answer 2

You could use std::string as internal buffer and use append() when adding new elements.

Thus adding strings or const char* would be trivial.

Adding/writing uint8 can be done with casting it to char, writing uint16 - to char* with length sizeof(uint16_t).

void write_uint16( uint16_t val )
{
    m_strBuffer.append( (char*)(&var), sizeof(val) );
}

Reading uint16:

uint16_t read_int16()
{
    return ( *(uint16_t*)(m_strBuffer.c_str() + m_nOffset) );
}

Answer 3

You appear to be attempting to print ten bytes out of the buffer when you've only added four, and thus you're running off the end of the vector. This could be causing your seg fault.

Also your printf is trying to print a character as an unsigned int with %x. You need to use static_cast<unsigned>(packet._buffer[i]) as the parameter.

Stylistically: Packet packet = Packet(); could potentially result in two objects being constructed. Just use Packet packet;

Generally try to avoid protected attributes (protected methods are fine) as they reduce encapsulation of your class.