Why doesn't my parse function return return all expected tokens?

Question

I wrote a program that reads a command line from the standard input, and pass it to a function that is supposed to parse it into tokens.

This is the parsing function:

char** parse_cmdline(char* cmdline) {
    char ** arr = malloc(10 * sizeof(char*));
    for (int i =0 ; i < 10; ++i)
        arr[i] = malloc(30 * sizeof(char));
    char * token = strtok(cmdline, " ");
    int i = 0;
    while(token != NULL) {
        if(i > 9) arr = realloc(arr, (i+10)*sizeof(char*) );
        arr[i] = token;
        token = strtok(NULL, " ");
        i++;
    }
    printf("flag1");
    return arr;
}

And this is how I am using it it main():

int main() {
    int status;
    pid_t pid;
    pid = fork();

    while(1) {      
        if(pid < 0) {
            status = -1;
            perror("Fork");
        } else if(pid == 0) {
            char* cmd;
            printf("$");
            if(fgets(cmd, sizeof cmd, stdin) == NULL) break;
            parse_cmdline(cmd);
        } else {
            if( waitpid(pid, &status, 0) != pid ) {
                status = -1;
            }
            break;
        }
    }


    return 0;
}

This is an example of input that I supply to my program:

ls l a

The expected output should be:

l

(that is, the second argument, printed by my parse function)

And literally nothing happens. Not even the printf("flag1"); prints. But if I remove the char ** commands and put the printf("%s", commands[0]); in the parse_cmdline function, everything works, except im not assigning the return. Why and how to fix it?

---

As requested, here's the entirety of my code:

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

char** parse_cmdline(char* cmdline) {
    char ** arr = malloc(10 * sizeof(char*));
    for (int i =0 ; i < 10; ++i)
        arr[i] = malloc(30 * sizeof(char));
    char * token = strtok(cmdline, " ");
    int i = 0;
    while(token != NULL) {
        if(i > 9) arr = realloc(arr, (i+10)*sizeof(char*) );
        arr[i] = token;
        token = strtok(NULL, " ");
        i++;
    }
    printf("%s\n", arr[1]);
    return arr;
}

Answer 1

This part looks strange - see comments inline:

char ** arr = malloc(10 * sizeof(char*));
for (int i =0 ; i < 10; ++i)
    arr[i] = malloc(30 * sizeof(char));     // Here you allocate memory
                                            // for holding a part of the command

char * token = strtok(cmdline, " ");
int i = 0;
while(token != NULL) {
    if(i > 9) arr = realloc(arr, (i+10)*sizeof(char*) );

    arr[i] = token;             // But here you overwrite the pointer value and
                                // and thereby create a memory leak

    token = strtok(NULL, " ");
    i++;
}

Perhaps you wanted to do a string copy instead - like:

strcpy(arr[i], token);   // Instead of arr[i] = token;

Further this line seems strange:

if(i > 9) arr = realloc(arr, (i+10)*sizeof(char*) );

You increase arr so that it can hold more char* but this time you don't allocate memory for the new strings as you did initially.

Answer 2

First, you are not allocating room for the command. Change the declaration of cmd to something like this:

char cmd[100];

Not allocating memory causes undefined behavior, and this (as well with proper usage of fgets fixes that). But you should also check from fgets() if 100 characters were enough:

if (strstr(cmd, "\n") == NULL) {
    /* the user typed more than 100 characters */
}

Because if they were not enough, then you are going to parse an incomplete command line and, the next time your loop iterates over the input data, it will parse more incomplete commands.

Finally, strtok returns pointers to the tokens in cmd, so all those characters array you allocated at the beginning of your parse function were memory leaks, because you replaced them with pointers from strtok inside the loop:

arr[i] = token;
/* this throws away the address of the 10-character array you allocated
 * at the beginning of the function. You can't free() that memory
 * anymore. Your program is "leaking" memory. */

Strictly speaking, by the way, you should be checking if realloc is returning a valid address or NULL. malloc too. It is unlikely that you'll have problems with this in such a small program, but it's the right practice.

You should also dispose of the parsed commands once you have used them. You allocate an array of pointers with both malloc and realloc, but you never free them in your program. Even though the program ends soon, while the program is running, that's memory leak. (Again, unlikely you'll see problems with a small program, but it's good practice.)