0

I currently have a Packet set up like so:

struct Packet {
    unsigned short sequenceNumber;
    unsigned short length;
    unsigned char control;
    unsigned char ack;
    unsigned short crc;
    unsigned char data[];
    Packet copy(const Packet& aPacket) {
        sequenceNumber = aPacket.sequenceNumber;
        length = aPacket.length;
        control= aPacket.control;
        ack = aPacket.ack;
        crc = aPacket.crc;
        memcpy (data, aPacket.data, aPacket.length);
    }
};

This packet gets converted into a string for encryption and then needs to be taken from its decrypted string form back to a Packet. I am able to do this fine for all of the variables except for the unsigned char data[]. I have tried the following with no success:

string data = thePack.substr(pos, thePack.length()-pos);
    unsigned char * cData = new unsigned char[data.length()];
    strcpy((char *)cData, data.c_str());
    memcpy(p.data, cData, data.length());

where data is the string representation of the data to be copied into the unsigned char [] and p is the Packet.

This gives the following from valgrind:

==16851== Invalid write of size 1

==16851==    at 0x4A082E7: strcpy (mc_replace_strmem.c:303)

Even though it cites strcpy as the source, it compiles and runs fine with just the memcpy line commented out.

I have also tried replacing memcpy with strcpy with the same result. Any ideas? I feel that it might be due to the fact that data may have not been initialized and there for not have any memory allocated to it, but I thought memcpy would take care of this.

Mateen Ulhaq
  • 24,552
  • 19
  • 101
  • 135
Jeff Schmitz
  • 1,087
  • 1
  • 13
  • 28

2 Answers2

3

You haven't specified the size of the data array.

unsigned char data[];

This is legal, but rather difficult to use. The data array will follow the rest of the Packet structure in memory, but the compiler doesn't know how much space to allocate for it. So you have to allocate the extra space yourself:

size_t datalen = thePack.length()-pos;
void* pbuffer = malloc( sizeof (Packet) + datalen + 1 );
Packet* p = new (pbuffer) Packet;
memcpy(p.data, &thePack[pos], datalen);
p.data[datelen] = 0;

What won't work is letting the compiler decide how big a Packet should be, either using new Packet or a local variable Packet p;. That will end up with no space reserved for data. And no, memcpy doesn't allocate memory.

A much cleaner solution would be to use a std::vector for your variable-sized data array.

Ben Voigt
  • 277,958
  • 43
  • 419
  • 720
1

The char[] you're allocating is one character too small -- you must leave room for the NULL byte at the end:

unsigned char * cData = new unsigned char[data.length() + 1];

Use the strcpy version to copy the string, so the NULL byte gets copied correctly. Although it might run OK without that +1, there's no guarantee, and sometimes it might crash.

Mateen Ulhaq
  • 24,552
  • 19
  • 101
  • 135
Ernest Friedman-Hill
  • 80,601
  • 10
  • 150
  • 186