0

I have some code that sends UDP sockets to a server. Currently I have a separate server code that I run locally which reads anything that's sent to it and writes back exactly what it receives.

The next step I need to make is to send and receive structs. I can send the struct fine, but when I receive it back from the server it's mashed up. Here is my code:

typedef struct {
 char first_part[4];
 char second_part;
 char third_part[2];
} Cloud;

Then in main:

char reply[BUFLEN], message[BUFLEN];
Cloud data;

strcpy(data.first_part, "test");
data.second_part = 'a';
strcpy(data.third_part, "hi");

printf("Size:%d\n", sizeof(data));

//This part seems to work---
char* package;
package = (unsigned char*)malloc(sizeof(data));
memcpy(package, &data, sizeof(data));
printf("Size:%d\n", strlen(package));
strcpy(message, package);

udp_send_receive(message,reply);
//---So the message is sent, and the string received by the server is correct.

memcpy(package, message, strlen(message));
printf("Package: %s\n",package); //-This is also correct

memcpy(&data, package, sizeof(data));

printf(data.first_part); //--This is incorrect

I would be really grateful if someone could explain what is going wrong here. I'm a little inexperienced with this sort of thing and I have been tasked with building a UDP server that communicates with another server, where the data is transferred in specific structures.

James
  • 3,957
  • 4
  • 37
  • 82
  • You say "this is incorrect". Apart from being very unsafe (it should be more like `printf("%.4s\n", data.first_part);`), what is wrong? What result do you get? How are you tracking how long a packet you receive from the other end? Have you written code to do hex dumps of the data packets you send and receive? Having such functions in your toolkit makes it easier to debug network code; you can look at the data and see whether it contains what you expected it to contain. Actually formatting the data is then not usually a big problem. Beware non-character data and big-endian vs little-endian. – Jonathan Leffler Sep 16 '13 at 14:52
  • 1
    Don't use structs as wire protocols. You are introducing dependencies on the hardware, the compiler, the compiler version, the compiler options, the surrounding #pragmas, ... Define a wire protocol separately as a specification, and write the code to marshall and unmarshall, or use a package such as XDR that will do that for you. – user207421 Sep 16 '13 at 23:24

3 Answers3

1

For the specific line you've identified as incorrect, you can't printf on data.first_part because it's not null-terminated. You would need to copy those four bytes (using e.g. memcpy) into another buffer of five bytes in length, and ensure that it's null-terminated.

However, the same applies to every one of your earlier calls to strXXX(), e.g. strcpy(data.first_part, "test"), strlen(package), strcpy(message, package) and strlen(message), etc - those functions must only ever be used on null-terminated strings, not arbitrary memory buffers, e.g.:

Cloud data, reply;

memcpy(data.first_part, "test", 4);  // not strcpy, it might overwrite .second_part
memcpy.second_part = 'a';
memcpy(data.third_part, "hi", 2); // not strcpy, it will overwrite the next value on the stack!

// no need to copy for transmission and receipt, BTW!
udp_send_receive(&data, &reply);

// copy reply data into a null-terminated string buffer
char tmp[5];
memcpy(tmp, reply.first_part, 4);
tmp[4] = '\0';

printf("%s\n", tmp); // should be fine!
Alnitak
  • 334,560
  • 70
  • 407
  • 495
  • Thanks, so I've increased the char[] lengths by 1, but not I get a problem earlier in my code. Only the first array is copied into `package`, do you have any suggestions for fixing this? – James Sep 16 '13 at 14:57
  • No, don't increase your `char[]` lengths - use the right functions to manipulate fixed-length buffers (e.g. `memXXX` instead of `strXXX`). – Alnitak Sep 16 '13 at 14:59
  • If you want to use memcpy, make sure client and server both send the same amount of data. More precisely, make sure your changes to struct cloud gets reflected at the server end too. – Vivek S Sep 16 '13 at 15:03
  • @VivekS if he uses `memcpy` he won't need to change `struct Cloud` ! – Alnitak Sep 16 '13 at 15:05
  • @Alnitak Yes, he doesn't, but isn't it tedious to assign each element separately instead of using memcpy to copy the entire data sent by the server ? – Vivek S Sep 16 '13 at 15:07
  • @VivekS who said anything about treating each element separately? The entire struct _should_ be treated as a single entity for transmission, but one does have to take precautions when *displaying* data that is not null-terminated, and that may require per-field processing. Changing the struct is not required, and from a network protocol point of view is undesirable. – Alnitak Sep 16 '13 at 15:10
  • Thanks @Alnitak, I've implemented your solution and everything seems to be working nicely. – James Sep 16 '13 at 18:41
1

In C, strings need a \0 ending.

So if you want to store "test", you need a char first_part[5] variable. The same for third_part which need a 3 bytes long for storing "hi" + '\0'.

Mali
  • 2,990
  • 18
  • 18
  • no, it's not necessary for the "on-the-wire" format to have that, it's OK for those to be fixed length strings. They only need zero-terminating when subsequently being treated as strings (e.g. for display) – Alnitak Sep 16 '13 at 14:51
  • @Alnitak: but if the code uses `strcpy(data.first_part, "test"); data.second_part = 'a'; strcpy(data.third_part, "hi");`, it copies 5 bytes into a 4 byte container, and 3 bytes into a 2 byte container, and happiness is not guaranteed. You're right that data need not be null terminated as it is sent over the wire. However, if you're dealing with such non-terminated data, you have to be very careful, and the code is not. – Jonathan Leffler Sep 16 '13 at 14:55
  • @JonathanLeffler indeed, but the right answer is not to change the structure, but to fix the code. – Alnitak Sep 16 '13 at 14:57
1

A few points about your code:

  1. This: strcpy(data.first_part, "test"); is a buffer overflow, since first_part is only 4 characters long. strcpy() will write an additional termination character, since it's copying the entire string. Use memcpy() if you don't want to work with 0-terminated strings.
  2. This: package = (unsigned char*)malloc(sizeof(data)); should drop the cast.
  3. Also, sizeof(data) is somewhat simpler written as sizeof data.
Community
  • 1
  • 1
unwind
  • 391,730
  • 64
  • 469
  • 606