I am working on a project including server-client communication. Our code doesn't work all the time, sometimes it works perfectly. But sometimes we either get a timeout or our buffer doesn't work properly. Thats why we want to implement malloc(). Do you think this could help? our code before malloc():

#include <stdio.h>
#include <stdlib.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <string.h>
#include <stdbool.h>
#include <unistd.h>

#define BUFFERSIZE 1024                                              
#define VERSION "VERSION 3.4\n"

#include "functions.h"

char buffer[BUFFERSIZE];

void resetBuffer(char *buffer) {
    memset((buffer), '\0', strlen(buffer));
}

void receiveAnswer(int sock) {
    resetBuffer(buffer);
    size_t length;
    bool x = true;

    while (x) {
        recv(sock, buffer, sizeof(buffer), 0);
        length = strlen(buffer);
        if (buffer[length-1]  == '\n') {
            x = false;
        }
    }

    if (buffer[0] == '-') {
        printf("Error: %s", buffer);
    } else {
        printf("%s\n ", buffer);
    }
}

void sendResponse(int sock, char *message) {
    resetBuffer(buffer);
    strcpy(buffer, message);

    bool x = true;
    size_t length;

    while(x) {
        send(sock, buffer, strlen(buffer), 0);
        length = strlen(buffer);
        if (buffer[length - 1] == '\n') {
            x = false;
        }
    }

    printf("Client: %s\n", buffer);
}

int performConnection(int sock, char *gameID) {
    receiveAnswer(sock);
    sleep(1);
    receiveAnswer(sock);
    sleep(1);
    sendResponse(sock, VERSION);
    sleep(1);
    receiveAnswer(sock);
    sleep(1);
    sendResponse(sock, gameID);
    sleep(1);
    receiveAnswer(sock);
    sleep(1);
    sendResponse(sock, "PLAYER \n");
    sleep(1);
    receiveAnswer(sock);

    resetBuffer(buffer);

    return 0;
}

Our code with malloc() doesn't work at all anymore:

#include <stdio.h>
#include <stdlib.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <string.h>
#include <stdbool.h>
#include <unistd.h>

#define BUFFERSIZE 1024
#define VERSION "VERSION 3.4\n"

#include "functions.h"

char *buffer;

void resetBuffer(char *buffer) {
    memset((buffer), '\0', strlen(buffer));
}

void receiveAnswer(int sock) {
    resetBuffer(buffer);
    size_t length;
    bool x = true;

    while (x) {
        recv(sock, buffer, sizeof(buffer), 0);
        length = strlen(buffer);
        if (buffer[length-1] == '\n') {
            x = false;
        }
    }

    if (buffer[0] == '-') {
        printf("Error: %s", buffer);
    } else {
        printf("%s\n ", buffer);
    }
}

void sendResponse(int sock, char *message) {
    resetBuffer(buffer);
    strcpy(buffer, message);

    bool x = true;
    size_t length;

    while (x) {
        send(sock, buffer, strlen(buffer), 0);
        length = strlen(buffer);
        if (buffer[length - 1] == '\n') {
            x = false;
        }
    }

    printf("Client: %s\n", buffer);
}

int performConnection(int sock, char *gameID) {
    buffer = (char *)malloc(sizeof(char) * BUFFERSIZE);
    receiveAnswer(sock);
    sleep(1);
    receiveAnswer(sock);
    sleep(1);
    sendResponse(sock, VERSION);
    sleep(1);
    receiveAnswer(sock);
    sleep(1);
    sendResponse(sock, gameID);
    sleep(1);
    receiveAnswer(sock);
    sleep(1);
    sendResponse(sock, "PLAYER \n");
    sleep(1);
    receiveAnswer(sock);

    resetBuffer(buffer);
    free(buffer);
    return 0;
}

Any help is appreciated! Best Enno

CodePudding user response：

resetBuffer(buffer); fails as it attempts strlen(buffer) on uninitialized data. This invokes undefined behavior (UB) as strlen() expects a pointer to a string.

Instead:

// resetBuffer(buffer);
memset(buffer, 0, sizeof(char)* BUFFERSIZE);
// or simply
memset(buffer, 0, BUFFERSIZE);

CodePudding user response：

this

void resetBuffer(char* buffer){
   memset((buffer), '\0', strlen(buffer));
}

is undefined behavior. It depends of the previous contents of buffer (strlen looks for 0 terminator)

you need to pass in a length (not got from strlen)

CodePudding user response：

Why are you bothering with the function reset_buffer() at all?

Each invocation appears immediately ahead of, for instance, strcpy(). The latter doesn't care if the buffer has be "reset". (Hopefully the buffer is large enough to hold what is being put into it.)

The last invocation appears immediately ahead of free(). Again, the content of the buffer is irrelevant.

More of a worry is that buffer[] has become *buffer... The sizeof(buffer) in the receive function is now telling recv there are 8 bytes available (the size of a pointer on your machine) to be filled, not the 1024 bytes of the "pre-malloc" version.

Change:

void receiveAnswer (int sock){
// ...
        recv(sock, buffer, sizeof(buffer), 0);

to

        recv(sock, buffer, BUFFERSIZE, 0);

This is not a MRE, so this "fix", while correct, may not be the entire solution.

CodePudding user response：

buffer does not necessarily contain a proper C string, hence using strlen to compute the number of bytes received is incorrect: use the return value of recv instead. Similarly, the buffer clearing function should either get the length as an argument or use BUFFER_SIZE.

Whether buffer is defined as a global array of bytes or allocated from the heap before use does not matter, and your code has nothing to do with implementing the malloc function.