CODEWITHSUNDEEP
cpointersmultidimensional-arraymemory-management
nunos
nunos

Reputation: 21409

Allocating memory for a array to char pointer

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.

Upvotes: 3

Views: 1658

Answers (7)

Jim Tshr
Jim Tshr

Reputation: 357

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++;
}

Upvotes: 0

Sridhar Iyer
Sridhar Iyer

Reputation: 2840

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");
    }

Upvotes: 0

Patrick Schl&#252;ter
Patrick Schl&#252;ter

Reputation: 11861

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.

Upvotes: 0

Eric Petroelje
Eric Petroelje

Reputation: 60529

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.

Upvotes: 0

Julien Lebosquain
Julien Lebosquain

Reputation: 41253

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.

Upvotes: 2

Steve Emmerson
Steve Emmerson

Reputation: 7832

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.

Upvotes: 0

JSBձոգչ
JSBձոգչ

Reputation: 41388

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.

Upvotes: 4

Related Questions