Proper way to send and receive buffer in Winsock

Question

I have a piece of code to send and receive buffers. Is this the right way to do it? And am I guaranteed that the full buffer will be sent and received?

receiving function:

#include <stdio.h>
#include <winsock2.h>
#include <ws2tcpip.h>
#include <direct.h>
#include <string.h>
#include <stdint.h>
static inline uint32_t ntohl_ch(char const* a)
{
 uint32_t x; memcpy(&x, a, sizeof(x));
 return ntohl(x);
}
char* recvStrBuffer(SOCKET s)
{
 int totalReceived = 0;
 int received = 0;
 // recv buffer size
 char b[sizeof(uint32_t)];
 int r = recv(s, b, sizeof(uint32_t), 0);
 if (r == SOCKET_ERROR)
 {
 printf("error recv\n");
 return NULL;
 }
 uint32_t bufferSize = ntohl_ch(&b[0]);
 //printf("bufferSize: %d\n", bufferSize);
 
 char* buff = (char*)malloc(sizeof(char) * bufferSize);
 while (totalReceived < bufferSize)
 {
 received = recv(s, buff + totalReceived, bufferSize - totalReceived, 0);
 if (received == SOCKET_ERROR)
 {
 printf("error receiving buffer %d\n", WSAGetLastError());
 return NULL;
 }
 totalReceived += received;
 //printf("received: %d\n", received);
 //printf("totalReceived: %d\n", totalReceived);
 }
 //printf("%s", buff);
 return buff;
}

sending function:

 #include <stdio.h>
 #include <winsock2.h>
 #include <ws2tcpip.h>
 #include <direct.h>
 #include <string.h>
 #include <stdint.h>
int sendStrBuffer(SOCKET s, char* buffer)
{
 // send buffer size
 int bufferSize = strlen(buffer);
 //printf("bufferSize: %d\n", bufferSize);
 uint32_t num = htonl(bufferSize);
 char* converted_num = (char*)&num;
 int res = send(s, converted_num, sizeof(uint32_t), 0);
 if (res == SOCKET_ERROR)
 {
 printf("error send\n");
 return SOCKET_ERROR;
 }
 int totalSent = 0;
 int sent = 0;
 while (totalSent < bufferSize)
 {
 sent = send(s, buffer + totalSent, bufferSize - totalSent, 0);
 if (sent == SOCKET_ERROR)
 {
 printf("error sending buffer\n");
 return SOCKET_ERROR;
 }
 totalSent += sent;
 //printf("sent: %d\n", sent);
 //printf("totalSent: %d\n", totalSent);
 }
}

And then in main (receiving part):

char* buffer;
buffer = recvStrBuffer(socket);
if (buffer == NULL) { printf("error %d\n", WSAGetLastError()); }
printf("%s", buffer);
free(buffer);

Main (sending part):

int r = sendStrBuffer(socket, totalBuffer);
if (r == SOCKET_ERROR) { printf("error %d\n", WSAGetLastError()); }

Answer 1

General Observations

The code is mostly readable and except in one case maintainable. The exception may be a copy-and-paste error.

Generally code isn't considered ready for review when it contains commented out debug statements such as

 //printf("bufferSize: %d\n", bufferSize);
 //printf("received: %d\n", received);
 //printf("totalReceived: %d\n", totalReceived);

Warning Messages

It would be better if you compiled using the -wall switch to catch all possible errors in the code. I compiled this with Visual Studio 2019 and got 2 warning messages, both messages indicate possible bugs:

warning C4018: '<': signed/unsigned mismatch 
warning C4715: 'sendStrBuffer': not all control paths return a value

The second warning is a problem that you definitely want to fix. You are not explicitly returning a value from sendStrBuffer when the function is successful. What gets returned is undefined and that definitely isn't a good thing, it may return SOCKET_ERROR or some other value that could cause problems in the calling program. The function should probably return zero if it is successful.

The first warning is on this line:

 while (totalReceived < bufferSize)

in recvStrBuffer(). The variable totalReceived is declared as a signed integer, the variable bufferSize is declared as an unsigned integer. It would be better if both variables were defined using the same type. It would be even better if both variables were defined as size_t since they represent a size. The type size_t is what is returned by the sizeof() operator. The size_t type is the largest unsigned integer value your system supports.

File and Program Organization

To make it easier to share values between the sending function and the receiving function it might be better to put both functions into a common library C source file and share the resulting object file between the sending program and the receiving program. There should be a dedicated header file that provides the function prototypes for both functions that the sending program and the receiving program include.

This file organization would make it easier to maintain the code because all the code for sending and receiving through the socket is in the same file.

Test for Possible Memory Allocation Errors

In modern high-level languages such as C++, memory allocation errors throw an exception that the programmer can catch. This is not the case in the C programming language. While it is rare in modern computers because there is so much memory, memory allocation can fail, especially if the code is working in a limited memory application such as embedded control systems. In the C programming language when memory allocation fails, the functions malloc(), calloc() and realloc() return NULL. Referencing any memory address through a NULL pointer results in undefined behavior (UB).

Possible unknown behavior in this case can be a memory page error (in Unix this would be call Segmentation Violation), corrupted data in the program and in very old computers it could even cause the computer to reboot (corruption of the stack pointer).

To prevent this undefined behavior a best practice is to always follow the memory allocation statement with a test that the pointer that was returned is not NULL.

Example of Current Code:

 char* buff = (char*)malloc(sizeof(char) * bufferSize);

Example of Current Code with Test:

 char* buff = (char*)malloc(sizeof(*buff) * bufferSize);
 if (!buff)
 {
 fprintf(stderr, "Malloc of buffer failed in recvStrBuffer\n");
 return NULL;
 }

Convention When Using Memory Allocation in C

When using malloc(), calloc() or realloc() in C a common convention is to sizeof(*PTR) rather sizeof(PTR_TYPE); this makes the code easier to maintain and less error prone, since less editing is required if the type of the pointer changes. See the example above.

Print Error Messages to stderr

There are 3 streams provided by stdio.h one, stdin is an input stream, two, stdout and stderr are output streams. Generally it is better to print error messages to stderr rather than stdout. When you redirect output to a file the two streams can be separated, and you can generate two files, one containing errors and the other containing program output. This helps when you are debugging or developing code.

One Statement Per Line

I don't know if this is a copy and paste error or if this the actual code in the program, but there should always be only one statement per line to make maintenance of the code easier.

static inline uint32_t ntohl_ch(char const* a)
{
 uint32_t x; memcpy(&x, a, sizeof(x));
 return ntohl(x);
}

This function should return size_t rather than uint32_t.

Use of the inline Keyword

The inline keyword is only a recommendation to the compiler; it may not do anything. Rather than use the inline keyword it is better to compile with the best optimization you can, generally -O3. The optimizing compilers will use inline code when that code fits into the cache memory whether you use the inline keyword or not.

• Y K: Thank you for this reply. Is it better if I use int32_t instead of size_t because size_t may differ on different machines?
• pacmaninbw: @YK size_t does differ on different systems, but it is based on the operating system as much as the machine. This code will only compile on Windows, so I don't think it is a problem. For size values size_t is preferred.