crash after memcpy: access violation reading location

Question

if (m_Connections[t].socket != INVALID_SOCKET)
{
    m_TCPResult = recv(m_Connections[t].socket, m_TCPRecvbuf, m_TCPRecvbuflen, 0);

    if (m_TCPResult > 0)
    {
        printf("[TCPReceive] Bytes (m_TCPResult) received from %d: %d\n", m_Connections[t].socket, m_TCPResult);

        // Deserialize the data
        Packets::MainPacket receivedData;
        memcpy(&receivedData, m_TCPRecvbuf, sizeof(receivedData));

        // Check the type and do something with the data
        CheckType(m_Connections[t].socket, receivedData);
    }
    else  
    {
        if (WSAGetLastError() != WSAEWOULDBLOCK)
            printf("TCPReceive error: %d\n", WSAGetLastError());
    }
}

So I have this piece of code. I need to do a memcpy() to convert the incoming data from winsock to a struct that can be read by the application. However, after the CheckType() method is done the application crashes giving me an Access Violation Reading Location error. I removed the memcpy() method once to check and then it worked fine (no crashes).

I have no idea what the problem might be. I've been searching on Google but haven't found anything useful that seems to be a solution to my problem

EDIT:

Some more info:

// in the header
char m_TCPRecvbuf[DEFAULT_BUFLEN];

// receivedData struct
struct MainPacket
{
    char type;
    int id;

    LoginData loginData;
    vector<PlayerData> playerData;
};

In case the answer is "in a header file", how are they defined? memcpy into an unknown data structure looks highly suspicious. — gnasher729, Oct 29 '14 at 10:34
CheckType is a simple switch that does something depending on the type of the struct (see definition in the question) — Dries, Oct 29 '14 at 10:37
should not it be like memcpy(&receivedData, m_TCPRecvbuf, sizeof(m_TCPRecvbuf)); — Ali Kazmi, Oct 29 '14 at 10:38

score 1 · Answer 1 · answered Oct 29 '14 at 11:05

1

You're writing over the vector when you do your memcpy. It's not a POD you can't initialize it via a memcpy, but instead have to use it's member functions to initialize it.

Think of it this way, the vector will have a pointer to the data it manages and a size_t indicating the size, at a minimum. You can't just initialize the pointer by memcpying a value you've received over the network. The pointer may make sense to the sender, but when you receive it all you've got is a pointer that's valid on the server, not in your application. Because of this the moment you try to use the vector you'll get undefined behaviour and will probably crash (if you're lucky).

Also, as a result of this sizeof doesn't work in the way you'd expect when applied to classes. For example, if you've got vector with 1,000 items in it then sizeof won't reflect this. What sizeof tells you is the combined size of all the member variables in the class definition (subject to padding). If our vector implementaton is just a pointer and a size_t then it'll probably be around 8 bytes on a 32bit platform, and 16 on a 64bit platform, regardless of how many items are in the vector.

What you need to do is encode information in the packet so that you can decode it. For example, rather than send a vector your packet should contain a field indicating the number of PlayerData instances, followed by the data for each player.

answered Oct 29 '14 at 11:05

Sean

60,939
11
97
136

And can the data of the possible PlayerData still be in a vector? – Dries Oct 29 '14 at 11:34
@Dries - you can store the data in a `vector`, you just can't `memcpy` into it. You'll need a slightly more verbose network packet structure which you pick apart and use to initialize your data. What you can't do it just send the bytes that make up `MainPacket` over a network and expect them to make sense on the receiver. – Sean Oct 29 '14 at 11:38
Ok, I think I get it. I'll add a new field with a number of playerData objects like you said. Do I then just do a for loop and fill in the new vector? I guess that should work as long as I have the number of objects and how large every object is? – Dries Oct 29 '14 at 11:40
You'll also need to look at how you serialize the `PlayerData` type. If it's got classes in then you'll have to apply the same rule. This means that your network packet will vary in size, so the normal approach is to have a field at the start of the network packet that indicates how many bytes make up the packet, so that you can correctly read the packet. – Sean Oct 29 '14 at 11:45
PlayerData currently holds another custom struct (to hold a 2d position). Damn, this might be much harder to do then I thought. Do you have an example somewhere possibly? – Dries Oct 29 '14 at 11:48
Not really! Anything that's a pointer is going to have to be written out as the thing pointed to, rather than the pointer. If the pointer is polymorphic then you'll need to prefix the serialized bytes with some sort of type indicator. If you've got a sequence of things (like a vector) then you'll need to write out a field indicating the number of things followed by the things, using the rules I've just mentioned. – Sean Oct 29 '14 at 11:52
It's usually a good idea to avoid C++ types when defining a network packet format as it can easily lead to the problems you've experienced. – Sean Oct 29 '14 at 11:53

crash after memcpy: access violation reading location

1 Answers1