Valgrind "Invalid write of size 8" error after malloc-CodePudding

So I want to parse a shell-like command and store it on a char** in order to pass it as an argument on the execv system call. In the function below the parsing and storing works correctly.

char ** input_delimit(char *command){            // 19
    char **retval;                               // 20
    const char space[2]=" ";                     // 21
    int i=0;                                     // 22
    retval=(char**)malloc(sizeof(char*));        // 23
    retval[i]=(char*)malloc(sizeof(char));       // 24
    retval[i]=strtok(command,space);             // 25
    while(retval[i]!=NULL){                      // 26
        i  ;                                     // 27
        retval[i]=(char*)malloc(sizeof(char));   // 28
        retval[i]=strtok(NULL,space);            // 29
    }                                            // 30
    return retval;                               // 31
}                                                // 32

When I run my code with Valgrind, I get the following error messages (although it does correctly what I want it to do). Why does this happen?

==824== Invalid write of size 8
==824==    at 0x109410: input_delimit (shell.c:28)
==824==    by 0x109493: execute_command (shell.c:35)
==824==    by 0x109600: main (shell.c:64)
==824==  Address 0x4a489d8 is 0 bytes after a block of size 8 alloc'd
==824==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==824==    by 0x1093A7: input_delimit (shell.c:23)
==824==    by 0x109493: execute_command (shell.c:35)
==824==    by 0x109600: main (shell.c:64)
==824==
==824== Invalid write of size 8
==824==    at 0x109439: input_delimit (shell.c:29)
==824==    by 0x109493: execute_command (shell.c:35)
==824==    by 0x109600: main (shell.c:64)
==824==  Address 0x4a489d8 is 0 bytes after a block of size 8 alloc'd
==824==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==824==    by 0x1093A7: input_delimit (shell.c:23)
==824==    by 0x109493: execute_command (shell.c:35)
==824==    by 0x109600: main (shell.c:64)
==824==
==824== Invalid read of size 8
==824==    at 0x109450: input_delimit (shell.c:26)
==824==    by 0x109493: execute_command (shell.c:35)
==824==    by 0x109600: main (shell.c:64)
==824==  Address 0x4a489d8 is 0 bytes after a block of size 8 alloc'd
==824==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==824==    by 0x1093A7: input_delimit (shell.c:23)
==824==    by 0x109493: execute_command (shell.c:35)
==824==    by 0x109600: main (shell.c:64)

CodePudding user response：

You dynamically allocated memory only for one pointer

   retval=(char**)malloc(sizeof(char*));

So this loop

   while(retval[i]!=NULL){
           i  ;
           retval[i]=(char*)malloc(sizeof(char));
           retval[i]=strtok(NULL,space);
   }

invokes undefined behavior.

Moreover these statements

retval[i]=(char*)malloc(sizeof(char));
retval[i]=strtok(command,space)

produce a memory leak because memory was allocated and its address was assigned to the pointer retval[i] and in the next statement the pointer was reassigned. So the address of the allocated memory is lost.

You need to use realloc for the pointer retval within the while loop.

CodePudding user response：

This ...

        retval=(char**)malloc(sizeof(char*));

... allocates enough space to store one char *.

This ...

        retval[i]=(char*)malloc(sizeof(char));

... allocates space for one char and assigns a pointer to it to retval[i]. That space is subsequently leaked when you overwrite that pointer with a different one:

        retval[i]=strtok(command,space);

But now comes the main problem. retval points to space large enough for one char *. Therefore, every iteration of this ...

                i  ;
                retval[i]=(char*)malloc(sizeof(char));
                retval[i]=strtok(NULL,space);

... overruns the bounds of the allocated space (not to mention allocating and immediately leaking another block). I would expect Valgrind to notice, and to emit exactly such a diagnostic as you describe.

Suggestions:

Start out by allocating space for at least two blocks, because you will need at least that for any valid command.
Skip those single-char allocations. Not only do they serve no useful purpose, they are actually harmful.
When you fill up the allocated space without reaching the end of the command, use realloc() to obtain space for more words. This implies that you must keep track of how much space you have already allocated. Example:
```
char *temp = realloc(retval, new_capacity * sizeof(*retval));
if (!temp) {
    // handle allocation failure ...
} else {
    retval = temp;
}        
```