Analyze valgrind output: "invalid free()"

Question

I have this strange error found by valgrind on a (stupid) authentication module which makes some on heap allocations.

 ==8009== Invalid free() / delete / delete[] / realloc()
 ==8009==    at 0x4C2A739: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
 ==8009==    by 0x40263F: authenticate (server_utils.c:109)
 ==8009==    by 0x401A27: main (server.c:240)
 ==8009==  Address 0x51f1310 is 0 bytes inside a block of size 18 free'd
 ==8009==    at 0x4C2A739: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
 ==8009==    by 0x402633: authenticate (server_utils.c:108)
 ==8009==    by 0x401A27: main (server.c:240)
 =8009== 
 ==8009== Invalid free() / delete / delete[] / realloc()
 ==8009==    at 0x4C2A739: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
 ==8009==    by 0x40264B: authenticate (server_utils.c:110)
 ==8009==    by 0x401A27: main (server.c:240)
 ==8009==  Address 0x51f1319 is 9 bytes inside a block of size 18 free'd
 ==8009==    at 0x4C2A739: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
 ==8009==    by 0x402633: authenticate (server_utils.c:108)
 ==8009==    by 0x401A27: main (server.c:240)
 ==8009== 
 ==8009== Invalid free() / delete / delete[] / realloc()
 ==8009==    at 0x4C2A739: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
 ==8009==    by 0x402657: authenticate (server_utils.c:111)
 ==8009==    by 0x401A27: main (server.c:240)
 ==8009==  Address 0x51f131e is 14 bytes inside a block of size 18 free'd
 ==8009==    at 0x4C2A739: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
 ==8009==    by 0x402633: authenticate (server_utils.c:108)
 ==8009==    by 0x401A27: main (server.c:240)
 ==8009== ...

From what I understand he says I do three invalid free() on lines 109-110-111. The error should be that I try to free up more space than is actually allocated, but I can not decide how much space deallocate. Also I do not understand why then it refers to line 108 (which is also a free()).

This is from the documentation (Invalid free):

Memcheck keeps track of the blocks allocated by your program with malloc/new, so it can know exactly whether or not the argument to free/delete is legitimate or not. Here, this test program has freed the same block twice. As with the illegal read/write errors, Memcheck attempts to make sense of the address freed. If, as here, the address is one which has previously been freed, you wil be told that -- making duplicate frees of the same block easy to spot.

You will also get this message if you try to free a pointer that doesn't point to the start of a heap block.

I can not really imagine how be in one of these cases.

/* Authentication through <user_id:password:flag>
 * Returns 1 on success, -1 if user_id doesn't exist, -2 on password mismatch 
 * On success sets access_permissions
 */
int authenticate(USR_PSW *received, int *access_permissions) {

/*Users file opening*/
FILE *fd;
fd = fopen(USERS_FILE, "r");
if (fd == NULL) {
    fprintf(stderr, "Users file opening error\n");
    exit(EXIT_FAILURE);
}

char *usr_psw_line = malloc(USR_SIZE + PSW_SIZE + 3 + 1);
if (usr_psw_line == NULL) {
    fprintf(stderr, "Dynamic alloc error\n");
    exit(EXIT_FAILURE);
}
char *usr_tok = malloc(USR_SIZE);
if (usr_tok == NULL) {
    free(usr_psw_line);
    fprintf(stderr, "Dynamic alloc error\n");
    exit(EXIT_FAILURE);
}
char *psw_tok = malloc(PSW_SIZE);
if (psw_tok == NULL) {
    free(usr_psw_line);
    free(usr_tok);
    fprintf(stderr, "Dynamic alloc error\n");
    exit(EXIT_FAILURE);
}
char *flg_tok = malloc(sizeof (char) *2);
if (flg_tok == NULL) {
    free(usr_psw_line);
    free(usr_tok);
    free(psw_tok);
    fprintf(stderr, "Dynamic alloc error\n");
    exit(EXIT_FAILURE);
}

/*Reading from file <user_id:password:flag> */
while (fgets(usr_psw_line, USR_SIZE - 1 + PSW_SIZE - 1 + 3 + 1, fd) != NULL) {
    usr_tok = strtok(usr_psw_line, ":");

    if (strcmp(usr_tok, received->user_id) == 0) {
        /*user_id found, password check*/
        psw_tok = strtok(NULL, ":");
        /*password match*/
        if (strcmp(psw_tok, received->password) == 0) {
            flg_tok = strtok(NULL, ":");
            *access_permissions = atoi(flg_tok);
            free(usr_psw_line); //108
            free(usr_tok);//109
            free(psw_tok);//110
            free(flg_tok);//111
            fclose(fd);
            return AUTHENTICATED;
        } else { //password unmatch
            free(usr_psw_line);
            free(usr_tok);
            free(psw_tok);
            free(flg_tok);
            fclose(fd);
            return INVALID_PSW;
        }
    } else {
        fseek(fd, 1, SEEK_CUR);
        continue;
    }

}
/*EOF Reached without match*/
free(usr_psw_line);
free(usr_tok);
free(psw_tok);
free(flg_tok);
fclose(fd);
return INVALID_USR;

}

The header file:

#ifndef __SERVER_UTILS_H__
#define __SERVER_UTILS_H__

#define USR_SIZE 9
#define PSW_SIZE 5 

/*Authentication data structure definition*/
typedef struct{
    char user_id[USR_SIZE]; // eg: user_123
    char password[PSW_SIZE]; // eg: a1b2
} USR_PSW;

#define NM_MAX_SIZE 17
#define NR_MAX_SIZE 11

/*System record structure*/
typedef struct{
    char first_name[NM_MAX_SIZE];
    char last_name[NM_MAX_SIZE];
    char number[NR_MAX_SIZE];
}TBOOK_RECORD;


/*Session status flags*/
#define NOT_AUTHENTICATED 0
#define AUTHENTICATED 1
#define INVALID_USR 2
#define INVALID_PSW 3

/*Persmissions flags*/
#define NO_PERM 0
#define READ_WRITE  1
#define O_WRITE  2
#define O_READ  3

/*Contains <user_id:password,flag> triplets stored in server */
 #define USERS_FILE "users.txt" 

/*Contains all telbook records <first_name:last_name:phone_number>*/
 #define RECORDS_FILE "records.txt"

/*Single records file line max length (before \n) */
 #define RECFILE_LINE_MAX_LEN  2*NM_MAX_SIZE+NR_MAX_SIZE+3+1;

/*Client operation*/
#define NO_OP 0
#define SEARCH_BY_NAME 1
#define SEARCH_BY_NUMBER 2
#define INSERT_RECORD 3

/* 
 * Returns number of bytes copied into buffer (excluding terminating null byte), 
 * or 0 on EOF, or -1 on error.
 * size_t : used for sizes of objects.
 * ssize_t: used for a count of bytes or an error indication (-1).
 */
 ssize_t readline(int fd, void *buffer, size_t n);

/* Authentication through <user_id:password:flag>
 * Returns 1 on success, -1 if user_id doesn't exist, -2 on password mismatch 
 * On success sets access_permissions
 */
int authenticate(USR_PSW *received, int *access_permissions);


int close_session(int sock_ds);

int search_by__(int op_code, TBOOK_RECORD *rc_rcvd, TBOOK_RECORD *rc_rspn
            , FILE *fd, char *file_line);

int insert_record(TBOOK_RECORD *rc_rcvd);

#endif  /* __SERVER_UTILS_H__ */

Answer 1

usr_tok = strtok(usr_psw_line, ":");
...
    psw_tok = strtok(NULL, ":");

You are overwriting your pointer to your malloced memory (thereby leaking it) with a pointer elsewhere. Then you are trying to free that pointer you received from elsewhere, which is (as valgrind says) invalid.

Answer 2

To add to the above answer just make sure you fclose (fp) in all the cases where you have a return statement

Answer 3

When you use strok() you are changing the pointer address. This way you may not point to the entire block when you call free(). You may use auxiliar pointers to store strok() return values and then you can use the original pointers to free() all the allocated space correclty.