4

Im learning about Winsock and Im having a strange issue when sending and receiving a simple string. Here's my code (pure C):

Client:



//...
//Declarations and stuff

//----------- SEND SOME DATA -------------------------------------------------

    char string1[] = "string-1";
    int bytes_sent = 0;

    bytes_sent = send(client_socket, string1, strlen(string1), 0);  

    printf("BYTES SENT: %i\n", bytes_sent);
    printf("\n-----------------------------------------------\n\n");

    system("pause");

//...

Server:



//...
//Declarations and stuff

//----------- START LISTENING FOR REQUESTS ------------------------------------

    SOCKET ClientSocket;

    #define BUFFER_SIZE 256

    int size;
    struct sockaddr_in client_info;
    char client_ip[16];
    char data_received[BUFFER_SIZE];    
    int bytes_received = 0; 

    listen(ListenSocket, SOMAXCONN);

    while(1){           

        ClientSocket = accept(ListenSocket, (struct sockaddr *)&client_info, &size);        
        strcpy(client_ip, inet_ntoa(client_info.sin_addr));     

        do{

            bytes_received = recv(ClientSocket, data_received, BUFFER_SIZE, 0);

            if(bytes_received > 0){
                printf("DATA RECEIVED FROM %s: %s (%i bytes)\n", client_ip, data_received, bytes_received);
            }


        }while(bytes_received > 0);

        printf("\n-----------------------------------------------\n\n");


    }

//...

The problem is that the server prints my string + some strange symbols (see the pic).

Strange symbols

Im using a stream socket. The example is very simple so I dont know what could be wrong. The problem disappears (server prints OK the string) if I randomly modify the string, or the server's buffer size, or both. The problem fixes if in the send() call i use sizeof() instead of strlen(). Im a little bit lost here. Please be kind if i have missed something, this is my very first post here. I can provide the whole code (its basically the winsock start and the socket definition) .

SergiGS
  • 51
  • 1
  • 6
  • There doesn't appear to be anything wrong with the code you posted, but it's not the whole program, so it could easily be wrong in context. It sounds like you *meant* to post the whole program but it got cut off. Also, you need to tell us what your "strange issue" is -- exactly what happened, and how does that differ from what you expected to happen? – zwol Jan 18 '13 at 17:23
  • 1
    Hint: looks like you don't send the terminating NUL byte. –  Jan 18 '13 at 17:28
  • @H2CO3: It is most likely the problem. Although not necessarily, depending on the receiving code. – netcoder Jan 18 '13 at 17:43
  • @netcoder Yep, pretty much. –  Jan 18 '13 at 17:44
  • Can you post the receiving code as well? – netcoder Jan 18 '13 at 17:45
  • @Zack aaah so sorry, i hadnt finished it and it got posted dont know why. Also, i tried to include a picture but i cant as im a new user. – SergiGS Jan 18 '13 at 18:17

3 Answers3

9

The data you send does not contain a terminating null character:

bytes_sent = send(client_socket, string1, strlen(string1), 0);

...because strlen does not count the terminating null. This isn't exactly the problem in itself, but rather is coupled with the fact that on the receiving side:

char data_received[BUFFER_SIZE];
// ...
bytes_received = recv(ClientSocket, data_received, BUFFER_SIZE, 0);

data_received is not initialized, and you can receive up to BUFFER_SIZE bytes. This means that since the data you send is not null-terminated:

  • If bytes_received < BUFFER_SIZE, the rest of data_received may not be initialized so accessing/printing would be undefined behavior. It's actually not 100% clear, as the documentation says:

    [...] calling recv will return as much data as is currently available—up to the size of the buffer specified [...]

    ...so it may mean that the rest of the buffer is left untouched.

  • If bytes_received == BUFFER_SIZE, there is no null-terminator, so printf will invoke undefined behavior by trying to print it, since it doesn't know where the string stops and will overrun the array.

The easiest ways to fix these are either to send a null terminator:

bytes_sent = send(client_socket, string1, strlen(string1)+1, 0); // +1 here
bytes_sent = send(client_socket, string1, sizeof(string1), 0);   // same as above

...or receive a byte less and put the null terminator on the receiving size:

bytes_received = recv(ClientSocket, data_received, BUFFER_SIZE-1, 0); // -1 here
data_received[bytes_received] = 0;

I'd personally go with the first.

netcoder
  • 66,435
  • 19
  • 125
  • 142
  • 1
    Yep, nice explanation, as for why in all the examples i've seen everybody uses simply strlen().. well in the server-side nobody printed the data, so i guess they didnt care about the null-terminator – SergiGS Jan 18 '13 at 18:56
  • @SergioGimeno: Right, printing the string is what makes the difference here. You don't have to send or receive a null terminator when dealing with sockets, but you'll need one if you try to print the data. – netcoder Jan 18 '13 at 18:58
  • All the commonly-used Internet protocols I can think of are either fully text-based and use CRLF terminators (HTTP, SMTP, FTP, etc) or they are fully binary-safe and use explicit length fields (TCP, SSL). – zwol Jan 18 '13 at 20:49
  • The second option to -1 from the buffer in the recv worked swimmingly for me for recv of chunked data. – jspacek Nov 23 '14 at 18:01
3

So the problem is that you're not sending the terminating NUL byte, but you seem to be treating the received string as a C string (i. e. you assume it's NUL-terminated). To fix it, instead of

bytes_sent = send(client_socket, string1, strlen(string1), 0);

write

bytes_sent = send(client_socket, string1, strlen(string1) + 1, 0);

Also, you mentioned that "nobody uses strlen(s) + 1" - perhaps because they pay attention to the number of bytes received at the receiving side.

-1

Try set the length of all your string data and then terminate the string in server like that:

bzero(data_received, sizeof(data_received));
bytes_received = recv(ClientSocket, data_received, BUFFER_SIZE, 0);
data_received[bytes_received] = '\0';

If this not resolve, perhaps the @H2CO3 can help you reading better what you ask :]

Alex
  • 450
  • 4
  • 14
  • If `bytes_received == BUFFER_SIZE`, you overrun `data_received`. Even if you'd do `bytes_received` it's not good, it would overwrite the last byte in the buffer. – netcoder Jan 18 '13 at 18:22