3

I'm having some difficulties extracting data from a buffer using memcpy.

First, I memcpy some variables into a buffer:

int l1_connect(const char* hostname, int port) {
    // Variables to be stored in the buffer
    char *msg = "Hi, I'm a message"; // strlen(msg) == 17
    uint16_t sender_id = htons(1); // sizeof(sender_id) == 2
    uint16_t packet_size = htons(sizeof(packet_size)+sizeof(sender_id)+strlen(msg)); // sizeof(packet_size) == 2

    // Checking values
    printf("l1_connect():\nsender_id: %d, packet_size: %d\n\n", ntohs(sender_id), ntohs(packet_size));
    // sender_id == 1, packet_size == 21

    // The buffer
    char buf[100];

    // Copying everything
    memcpy(&buf, &sender_id, sizeof(sender_id));
    memcpy(&buf+sizeof(sender_id), &packet_size, sizeof(packet_size));
    memcpy(&buf+sizeof(sender_id)+sizeof(packet_size), &msg, strlen(msg));

    // Passing buf to another function
    int bytes_sent = l1_send(1, buf, sizeof(buf));
}

I then try to extract that data (checking, before sending over UDP socket):

int l1_send( int device, const char* buf, int length ) {
    // Variables in which to store extracted data
    uint16_t id = 0;
    uint16_t size = 0;
    char msg[50];

    memcpy(&id, &buf, sizeof(id));
    memcpy(&size, &buf+sizeof(id), sizeof(size));

    int remaining = ntohs(size) - (sizeof(id) + sizeof(size));
    printf("l1_send():\nremaining: %d\n", remaining); // -37041

    // memcpy-ing with correct(?) offset
    memcpy(&msg, &buf+sizeof(id)+sizeof(size), 50);

    msg[49] = '\0';

    printf("id: %d\n", ntohs(id));      // 8372
    printf("size: %d\n", ntohs(size));  // 37045
    printf("msg: %s\n", msg);           // ��$_�

    return 0; // For now
}

As you can see, the values aren't quite what I'm expecting. Can anyone tell me what I'm doing wrong?

Plasma
  • 1,903
  • 1
  • 22
  • 37

2 Answers2

6

Your pointer math is incorrect. You're using &buf where you should just be using buf. If this doesn't explain what is wrong, nothing else I can say will:

#include <stdio.h>

int main(int argc, char **argv)
{
    char buff[100];
    printf("buff : %p\nbuff+10 : %p\n&buff+10 : %p\n", buff, buff+10, &buff+10);
    return 0;

}

Output (varies by platform, obviously)

buff : 0xbf87a8bc
buff+10 : 0xbf87a8c6
&buff+10 : 0xbf87aca4

See it live. The math you're doing is incrementing by type, which for &buf is a pointer to array of 100 chars; not a simple char address. Therefore, &buff + 10 (in my sample) says "give me the 10th array of 100 chars from where I am now.". The subsequent write is invoking undefined behavior as a consequence.

Valgrind is your buddy here, btw. It would have caught this in a heartbeat.

Update

May as well fill in the entire gambit while I'm here. This is also wrong in l1_send:

memcpy(&id, &buf, sizeof(id));
// this------^

and the subsequent other areas you're using it in that function. You're taking the address of a parameter pointer, not the value within it. I'm confident you need buf there as well.

WhozCraig
  • 65,258
  • 11
  • 75
  • 141
  • You are quite right! I truly do not understand when to use ampersands, haha. Thanks a bunch! – Plasma Feb 17 '14 at 21:55
  • 1
    its helpful to remember that `&` always evaluates as an address typed to the variable it is preceding. Arrays are not an exception to this. Neither are pointers (as is the case in the Update I added). An array's "value" (when used in an naked expression) is the address of its 0-element, and its type is a pointer-to-element; not pointer to array of elements. I know it can seem a little daunting, but work with it awhile, it will come. – WhozCraig Feb 17 '14 at 21:59
3

Try this:

memcpy(buf, &sender_id, sizeof(sender_id));
memcpy(buf + sizeof(sender_id), &packet_size, sizeof(packet_size));
memcpy(buf + sizeof(sender_id) + sizeof(packet_size), msg, strlen(msg));

To help you understand what is wrong with your code, you can read this.

Related: Pointer math vs. Array index

Community
  • 1
  • 1
Heeryu
  • 852
  • 10
  • 25