Help with malloc and free: Glibc detected: free(): invalid pointer

Question

I need help with debugging this piece of code. I know the problem is in malloc and free but can't find exactly where, why and how to fix it. Please don't answer: "Use gdb" and that's it. I would use gdb to debug it, but I still don't know much about it and am still learning it, and would like to have, in the meanwhile, another solution.

Thanks.

#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#include <unistd.h>
#include <string.h>
#include <sys/wait.h>
#include <sys/types.h>

#define MAX_COMMAND_LENGTH  256
#define MAX_ARGS_NUMBER     128
#define MAX_HISTORY_NUMBER  100

#define PROMPT ">>> "

int num_elems;

typedef enum {false, true} bool;

typedef struct {
    char **arg;     
    char *infile;   
    char *outfile;  
    int background; 
} Command_Info;

int parse_cmd(char *cmd_line, Command_Info *cmd_info)
{
    char *arg;
    char *args[MAX_ARGS_NUMBER];    

    int i = 0;
    arg = strtok(cmd_line, " ");
    while (arg != NULL) {
        args[i] = arg;
        arg = strtok(NULL, " ");
        i++;
    }

    num_elems = i;precisa em free_mem
    if (num_elems == 0)
        return 0;

    cmd_info->arg = (char **) ( malloc(num_elems * sizeof(char *)) );
    cmd_info->infile = NULL;
    cmd_info->outfile = NULL;
    cmd_info->background = 0;

    bool b_infile = false;
    bool b_outfile = false;

    int iarg = 0;
    for (i = 0; i < num_elems; i++)
    {                   
        if ( !strcmp(args[i], "<") )
        {               
            if ( b_infile || i == num_elems-1 || !strcmp(args[i+1], "<") || !strcmp(args[i+1], ">") || !strcmp(args[i+1], "&") )
                return -1;                      

            i++;
            cmd_info->infile = malloc(strlen(args[i]) * sizeof(char));
            strcpy(cmd_info->infile, args[i]);
            b_infile = true;
        }

        else if (!strcmp(args[i], ">"))
        {
            if ( b_outfile || i == num_elems-1 || !strcmp(args[i+1], ">") || !strcmp(args[i+1], "<") || !strcmp(args[i+1], "&") )
                return -1;

            i++;    
            cmd_info->outfile = malloc(strlen(args[i]) * sizeof(char));
            strcpy(cmd_info->outfile, args[i]);
            b_outfile = true;
        }

        else if (!strcmp(args[i], "&"))
        {
            if ( i == 0 || i != num_elems-1 || cmd_info->background )
                return -1;

            cmd_info->background = true;
        }

        else
        {
            cmd_info->arg[iarg] = malloc(strlen(args[i]) * sizeof(char));
            strcpy(cmd_info->arg[iarg], args[i]);
            iarg++;
        }
    }

    cmd_info->arg[iarg] = NULL; 

    return 0;
}


void print_cmd(Command_Info *cmd_info)
{
    int i;  
    for (i = 0; cmd_info->arg[i] != NULL; i++)
        printf("arg[%d]=\"%s\"\n", i, cmd_info->arg[i]);
    printf("arg[%d]=\"%s\"\n", i, cmd_info->arg[i]);    
    printf("infile=\"%s\"\n", cmd_info->infile);
    printf("outfile=\"%s\"\n", cmd_info->outfile);
    printf("background=\"%d\"\n", cmd_info->background);
}

void get_cmd(char* str)
{
    fgets(str, MAX_COMMAND_LENGTH, stdin);
    str[strlen(str)-1] = '\0';
}

pid_t exec_simple(Command_Info *cmd_info)
{
    pid_t pid = fork();

    if (pid < 0)
    {
        perror("Fork Error");
        return -1;
    }

    if (pid == 0)
    {
        if ( (execvp(cmd_info->arg[0], cmd_info->arg)) == -1)
        {
            perror(cmd_info->arg[0]);
            exit(1);
        }
    }

    return pid;
}

void type_prompt(void)
{
    printf("%s", PROMPT);
}

void syntax_error(void)
{
    printf("msh syntax error\n");
}

void free_mem(Command_Info *cmd_info)
{
    int i;
    for (i = 0; cmd_info->arg[i] != NULL; i++)
        free(cmd_info->arg[i]);
    free(cmd_info->arg);
    free(cmd_info->infile);
    free(cmd_info->outfile);
}

int main(int argc, char* argv[])
{
    char cmd_line[MAX_COMMAND_LENGTH];
    Command_Info cmd_info;
    //char* history[MAX_HISTORY_NUMBER];

    while (true)
    {   
        type_prompt();

        get_cmd(cmd_line);

        if ( parse_cmd(cmd_line, &cmd_info) == -1)
        {
            syntax_error();
            continue;
        }

        if (!strcmp(cmd_line, ""))
            continue;

        if (!strcmp(cmd_info.arg[0], "exit"))
            exit(0);

        pid_t pid = exec_simple(&cmd_info);

        waitpid(pid, NULL, 0);

        free_mem(&cmd_info);
    }

    return 0;
} 

Answer 1

When you are dynamically allocating memory for cmd_info->infile as:

cmd_info->infile = malloc(strlen(args[i]) * sizeof(char));

you are not allocating space for the terminating null char.

Same is the case with allocation for cmd_info->outfile

When you allocate space for n char and copy a string of length n into it, I think that overwrites the metadata that malloc maintains at the end of the array and this bug shows up when you call free to deallocate the memory as free does not find that meta data.

EDIT:

Change:

num_elems = i;

to

num_elems = i+1;

Since you are marking the end of the arguments with NULL

cmd_info->arg[iarg] = NULL;

you need to allocate the space for this.

Answer 2

Since strings in C are null-terminated, their actuall size in memory is length+1, so instead of

cmd_info->infile = malloc(strlen(args[i]) * sizeof(char));

You should have

cmd_info->infile = malloc((strlen(args[i])+1) * sizeof(char));

EDIT: As Aeth said, you need to change every single occurence of malloc to contain space for that additional null character:

cmd_info->arg = (char **) ( malloc(num_elems * sizeof(char *)) ); //this one can stay, since it determines number of strings, not string length
cmd_info->outfile = malloc((strlen(args[i])+1) * sizeof(char));
cmd_info->arg[iarg] = malloc((strlen(args[i])+1) * sizeof(char));

Answer 3

1. You need to allocate an extra char for each of your strings to handle the terminating null.

   cmd_info->arg[iarg] = malloc((strlen(args[i])+1) * sizeof(char));

2. You need to allocate an additional char* in the cmd_info->arg array. This extra element will store the NULL that signifies the end of the array of arguments.

   cmd_info->arg = (char **) ( malloc((num_elems+1) * sizeof(char *)) );

---

I have confirmed on my system that the program successfully frees all its memory without error after both of the changes listed were made.

Answer 4

In general, this error is usually the result of something writing data outside a malloc()'d block (off the end or before the beginning). This can corrupt the memory allocator's internal bookkeeping structures.

Others have already pointed out the particular problem in your code. In cases where it's more deeply hidden, I have found Valgrind to be useful for debugging. At the expense of noticable code slowdown, it is able to detect illegal memory accesses (in the form of "invalid reads" and "invalid writes") at a very fine-grained level. Memory debuggers such as dmalloc can also be helpful, and don't impose nearly as much overhead, but in my experience aren't quite as good as Valgrind for finding everything.

Valgrind, in its 'memcheck' mode, will output memory access errors with a stack trace of where in the program they occurred. Usually, when I have an "invalid pointer" error in free(), it will be preceeded at some point by an invalid write which memcheck will find.