Allocating memory for a array to char pointer

Question

The following piece of code gives a segmentation fault when allocating memory for the last arg. What am I doing wrong? Thanks.

    int n_args = 0, i = 0;
    while (line[i] != '\0')
    {
        if (isspace(line[i++]))
            n_args++;
    }

    for (i = 0; i < n_args; i++)
        command = malloc (n_args * sizeof(char*));

    char* arg = NULL;
    arg = strtok(line, " \n");
    while (arg != NULL)
    {
        arg = strtok(NULL, " \n");
            command[i] = malloc ( (strlen(arg)+1) * sizeof(char) );
        strcpy(command[i], arg);
        i++;
    }

Thanks.

Answer 1

You don't reset the value of i after the for loop, so i is equal to n_args when you reach the bottom block. Trying to access command[i] at that point accesses uninitialized memory and segfaults.

The real lesson here is not to reuse variables in this manner without a good reason. Your code will be more robust and easier to read if you use something other than i in the middle for loop.

Answer 2

For a line comprising two arguments separated by a single space, n_args will be 1 rather than 2. This is probably not what you want.

Answer 3

for (i = 0; i < n_args; i++)
        command = malloc (n_args * sizeof(char*));

should become just

command = malloc (n_args * sizeof(char*))

because you just want to alloc an array of n_args elements, and

while (arg != NULL)
    {
        arg = strtok(NULL, " \n");
        command[i] = malloc ( (strlen(arg)+1) * sizeof(char) );
        strcpy(command[i], arg);
        i++;
    }

should become:

arg = strtok(NULL, " \n");
while (arg != NULL) {
    command[i] = malloc ( (strlen(arg)+1) * sizeof(char) );
    strcpy(command[i], arg);
    i++;
    arg = strtok(NULL, " \n");
}

to avoid strlen on a null pointer.

Answer 4

I think you have a few funny things going on here (if I'm reading this correctly).

This block:

for (i = 0; i < n_args; i++)
    command = malloc (n_args * sizeof(char*));

Should be this:

    command = malloc (n_args * sizeof(char*));

No need to reallocate command over and over again.

As for the seg fault though, could be because you are re-using the i variable without resetting it to zero again first.

Answer 5

You're throwing away your first arg? Is that intentional? In case it isn't

int n_args = 1;     /* We need one element more than spaces */
int i = 0;
while (line[i])
{
    if (isspace(line[i++]))
        n_args++;
}

command = malloc (n_args * sizeof(char*));

char* arg = NULL;
arg = strtok(line, " \n");
i = 0;        /***** You forgot to reset that value, that was your segfault !!! */
while (arg)
{
    command[i++] = strdup(arg);  /* It does your malloc/strlen/strcpy */
    arg = strtok(NULL, " \n");
}

You forgot to reset your i index, which reaches outside your allocated array in your code.

Answer 6

Try arranging this loop properly:

 while (arg != NULL)
    {
        arg = strtok(NULL, " \n");
            command[i] = malloc ( (strlen(arg)+1) * sizeof(char) );
        strcpy(command[i], arg);
        i++;
    }

the line "arg=strtok..." does 2 things wrongs:

1. Skips the first argument.
2. Doesn't check the return code, so if arg==NULL then strlen(arg) will SEGFAULT.

Do this instead:

 while (arg != NULL)
    {
        command[i] = malloc ( (strlen(arg)+1) * sizeof(char) );
        strcpy(command[i], arg);
        i++;
        arg = strtok(NULL, " \n");
    }

Answer 7

It is hard to figure out what you are trying to do.

It looks like you are looking at the number of spaces in the command line to see how many command line arguments you have. Are all of your command line args a single character? The malloc is only reserving enough space for one character per arg.

If your args are just one char each:

command = malloc(strlen(line));
i = 0;
j = 0;
while(line[j]) {
   if(!isspace(line[j])){
      command[i++] = line[j];
   }
   j++;
}