Command manipulation functions for a toy shell program

Question

I am writing a small program that is supposed to act as a shell of some sorts. It operates off of a few basic structures, one of them being a command, a command_list, and a command_history.

typedef struct command command;

typedef struct
{
    char** list;
    int len;
} command_history;

typedef struct 
{
    command** list;
    int len;
    command_history* history;
} command_list;

typedef struct command
{
    char* alias;
    void (*function)(command_list*);
} command;

I have written functions to add new commands, add new history, find aliases in a list and return its index, among other things, but I have a feeling that the command for adding commands, addcommand is leaking memory. I am not very experienced with C and this is just an experiment to acquaint me with pointers. I would appreciate some feedback on the code I have written and how I can improve it.

Command manipulation functions:

void addcommand(char* str, void* fun, command_list* srclist)
{
    printdebug("Adding command \"%s\" pointing to function at %p in command list located at %p... ", str, fun, srclist);
    srclist -> list = realloc(srclist -> list, sizeof(command*) * (srclist -> len + 1));
    srclist -> list[srclist -> len] = malloc(sizeof(command));
    srclist -> list[srclist -> len] -> alias = malloc(sizeof(char) * (strlen(str) + 1));
    srclist -> list[srclist -> len] -> function = malloc(sizeof(void*)); // problems here and |
    memcpy(srclist -> list[srclist -> len] -> alias, str, sizeof(char) * (strlen(str) + 1)); //       |
    srclist -> list[srclist -> len] -> function = fun; //                                    <|
    srclist -> len++;
    printdebug("Command added.\n");
}

void addhistory(char* str, command_list* srclist)
{
    printdebug("Adding \"%s\" to history list located at %p...", str, srclist -> history);
    srclist -> history -> list = realloc(srclist -> history -> list, sizeof(char*) * (srclist -> history -> len + 1));
    srclist -> history -> list[srclist -> history -> len] = malloc((strlen(str) + 1) * sizeof(char));
    memcpy(srclist -> history -> list[srclist -> history -> len], str, (strlen(str) + 1) * sizeof(char));
    srclist -> history -> len++;
    printdebug(" String added to history.\n");
}

void renamecommand(char* str, command* srccom, command_list* srclist)
{
    if(!srccom -> alias || !srccom -> function)
        return;
    printdebug("Renaming \"%s\" to \"%s\" in command list located at %p... ", srccom -> alias, str, srclist);
    free(srccom -> alias);
    srccom -> alias = malloc((strlen(str) + 1) * sizeof(char));
    memcpy(srccom -> alias, str, (strlen(str) + 1) * sizeof(char));
    printdebug("Renamed command.\n");
}

void deletecommand(command* srccom)
{
    printdebug("Deleting command \"%s\" pointing to function %p located at %p... ", srccom -> alias, srccom -> function, srccom);
    srccom -> alias = "[REMOVED]";
    srccom -> function = NULL;
    printdebug("Command deleted.\n");
}

command_list* newcommandlist()
{
    printdebug("Making new command list... ");
    command_list* new_list = malloc(sizeof(command_list));
    new_list -> list = NULL;
    new_list -> len = 0;
    new_list -> history = malloc(sizeof(command_history));
    new_list -> history -> list = NULL;
    new_list -> history -> len = 0;
    printdebug(" Made new command list located at %p.\n", new_list);
    return new_list;
}

Answer 1

You indeed have a memory leak in your addcommand function, at this line:

srclist -> list[srclist -> len] -> function = malloc(sizeof(void*));

The reason is that the pointer that you obtained from malloc stored into function gets overwritten later by the following line:

srclist -> list[srclist -> len] -> function = fun;

After this point, the pointer to the memory returned by malloc is lost and you cannot free it, hence the memory leak.

In fact, you don't need to malloc anything for that function, since the memory for the function pointer was already allocated by this line:

srclist -> list[srclist -> len] = malloc(sizeof(command));

The command struct already contains the memory for the field function. So you can just set function to the fun pointer.

Note also that the fun is not a void pointer, but rather a function pointer, so you should declare it as void (*fun)(command_list *) instead of void *fun.

Another comment: why do you keep a command** list instead of command* list? You are currently allocating memory twice, once for the command * pointer, and then for the actual command struct. A simpler (and less memory-hungry) solution would be to have:

typedef struct 
{
    command* list; // changed command **list to command *list
    int len;
    command_history* history;
} command_list;

and adding a new command in the list would look like:

void addcommand(char* str, void (*fun)(command_list*), command_list* srclist)
{
    printdebug("Adding command \"%s\" pointing to function at %p in command list located at %p... ", str, fun, srclist);
    srclist -> list = realloc(srclist -> list, sizeof(command) * (srclist -> len + 1));
    srclist -> list[srclist -> len] -> alias = malloc(sizeof(char) * (strlen(str) + 1));
    memcpy(srclist -> list[srclist -> len] -> alias, str, sizeof(char) * (strlen(str) + 1)); //       |
    srclist -> list[srclist -> len] -> function = fun; //                                    <|
    srclist -> len++;
    printdebug("Command added.\n");
}

Additionally, you should check the return value for malloc and realloc. They may return a NULL pointer if there is insufficient memory, and the code that dereferences these pointers will cause a segmentation fault and crash the program.