Valgrind: Invalid read / write of size 1

Question

I am trying to build a very basic web server using C. I have solved all of the problems reported by valgrind except this one. This is the relevant piece of code that causes it. I have added x>> to the lines that valgrind suggests:

/* Set the response header. */
char *response_header = "HTTP/1.1 404 Not Found\r\n"
        "Content-Type: text/html\r\n"
        "Connection: close\r\n"
        "\r\n";

/* Try to load the 404 message. Return 0 if failed. */
int response_body_size = 0;
char *response_body = read_file(FILE_404, &response_body_size);
if (response_body == NULL) {
    return 0;
}
terminate_string(response_body, response_body_size);

/* Allocate space to merge the header and body and merge them. Return 0 if failed. */
1>> char *response = (char *) malloc(sizeof(char) * (strlen(response_header) + strlen(response_body)));
if (response == NULL) {
    return 0;
}
strcpy(response, response_header);
2,3>> strcat(response, response_body);

/* Return the response. */
4>> write(connection_fd, response, strlen(response));

terminate_string():

/* Adds the terminating character to the specified string. */
void terminate_string(char *str, int length) {
5>> str[length] = '\0';
}

read_file():

/* Reads the specified file and returns its contents. Will return NULL if could not read. */
/* Assumes the filename starts with a / character! */
void* read_file(char *filename, int *file_size) {
    /* Open the file in binary read mode. Return NULL if failed. */
    FILE *file = fopen(filename + 1, "rb");
    if (file == NULL) {
        return NULL;
    }

    /* Get the size of the file. Return NULL if failed.*/
    struct stat fs;
    if (fstat(fileno(file), &fs) == -1) {
        return NULL;
    }

    *file_size = fs.st_size;

    /* Read the contents to a string. */
    void* result = NULL;
6>> result = (void *) malloc (fs.st_size);
    /* Return NULL if malloc failed. */
    if (result == NULL) {
        return NULL;
    }
    fread(result, 1, *file_size, file);

    /* Close the file. */
    fclose(file);

    return result;
}

Valgrind report:

1: Invalid read of size 1 [PID: 3896]
2: Invalid read of size 1 [PID:3896]
3: Invalid write of size 1 [PID:3896]
4: Invalid read of size 1 [PID:3896]
5: Invalid write of size 1 [PID:3896]
6: Address 0x541bbb4 is 0 bytes after a block of size 4 alloc'd [PID: 3896]

For some reason, the read_file() function returns 2 extra characters. This appears to be solved by terminate_string() but obviously valgrind knows something I don't. I have been looking at this code for the past 3 hours while reading every possible solution I found online and I didn't manage to figure it out. I am very new to dynamic memory allocation in C so it might be very obvious to someone with experience.

What is the problem here and how could I fix it?

Also, why is read_file() returning 2 bytes more than the file contains?

Answer 1

Firstly, well done for using valgrind.

At point 6, you do:

result = (void *) malloc (fs.st_size);

I suggest you do:

size_t sz = fs.st_size;
result = malloc (sz+1); /* no need to cast return of malloc() */
((char *)result)[sz] = 0; /* zero terminate it */

As the problem you have is that you have malloc'd exactly sufficient room for the file and the body, but not the terminating NUL.

Your terminate_string idea is broken as that writes beyond the end of response_body. If the response body is zero terminated you don't need that, so can drop it.

For a similar reason, you want:

char *response = malloc(sizeof(char) *
                 (strlen(response_header) + strlen(response_body) + 1));

+1 being for the NUL.

However, there is a bigger problem: HTTP documents themselves can contain \0, i.e. zero bytes, in which case you cannot strlen() them. A better fix would therefore be to write the response header, then write the response body, and simply keep the size of the body around as an integer. I've explained what the problem is above as it's important you know.