Why am I getting free(): invalid pointer?

Question

I am currently reading Linux System Programming by Robert Love and am stuck on the read() example that takes care of all five error cases. I am getting an free(): invalid pointer error. I am assuming that it has something to do with advancing the buffer in case the read is not finished.

It works if I store the offset and return the pointer to its original position. This is not mentioned in the book. Is there a better approach?

#include <stdio.h>
#include <malloc.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <string.h>

int main()
{

    int fd;

    if( (fd = open("someFile.txt", O_CREAT | O_WRONLY | O_TRUNC, 0664)) < 0)
        perror("open for write");

    char * text =  "This is an example text";
    write(fd, text, strlen(text));

    close(fd);

    if( (fd = open("someFile.txt", O_RDONLY)) < 0)
        perror("open");

    char *buf; 

    if( (buf = (char *) calloc(50, sizeof(char))) == NULL )
        perror("calloc");

    int len = 50;
    ssize_t ret;

    /* If I store the offset in a variable it works */

    off_t offset = 0;

    while (len != 0 && (ret = read (fd, buf, len)) != 0) {
        if (ret == -1) {
            if (errno == EINTR)
                continue;
            perror ("read");
            break;
        }

        len -= ret;
        buf += ret;
        offset += ret;  // Offset stored here

    }

    if( close(fd) == -1 )
        perror("close");

    buf -= offset;     // Here I return the pointer to its original position
    free(buf);

    return 0;
}

Answer 1

There are multiple bugs in this code.

First, perror is not being used correctly, as it only prints an error -- there should also be code here to abort on errors, so subsequent code doesn't try to use results from operations that failed.

Secondly, only the result from calloc can be given to free. The result is saved in buf, but then later code changes the value of buf and tries to free the changed value. Storing the changes in offset should fix this, but this is an error prone solution at best. If you have multiple code paths that modify buf, you have to make sure every one of those also modify offset in the same way.

A better approach would be to not modify buf, and instead use a second pointer variable in the read that is initialized to the value of buf and then gets modified after each read.

As pointed out, the number given to calloc is different than the number len is initialized to. This is a perfect example of misuse of a magic number. Both the 20 and 50 should be replaced with the same symbol (variable or constant or #define) so that you don't get a buffer overrun error.