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