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.

JSBձոգչ · Accepted Answer · 2010-05-25 18:36:05Z

4

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.

answered May 25, 2010 at 18:36

JSBձոգչ

41.6k19 gold badges106 silver badges173 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Julien Lebosquain · Accepted Answer · 2010-05-25 18:37:54Z

2

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.

answered May 25, 2010 at 18:37

Julien Lebosquain

41.4k9 gold badges108 silver badges120 bronze badges

1 Comment

Patrick Schlüter Over a year ago

Putting strtok(NULL at the end avoid also skipping the first parameter.

Steve Emmerson · Accepted Answer · 2010-05-25 18:36:09Z

0

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.

answered May 25, 2010 at 18:36

Steve Emmerson

7,8426 gold badges36 silver badges59 bronze badges

Comments

Eric Petroelje · Accepted Answer · 2010-05-25 18:40:04Z

0

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.

answered May 25, 2010 at 18:40

Eric Petroelje

60.7k9 gold badges133 silver badges178 bronze badges

Comments

Patrick Schlüter · Accepted Answer · 2010-05-25 18:53:49Z

0

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.

edited May 25, 2010 at 18:53

answered May 25, 2010 at 18:43

Patrick Schlüter

12k1 gold badge45 silver badges50 bronze badges

1 Comment

Patrick Schlüter Over a year ago

I add it as comment, don't want to reedit my answer. Minor quibble, it's bad style to reuse the variable for different purposes, you reuse i in 3 different places, in 2 of them it indexes the same thing so it's OK in my eyes. The first use is different, index on line which is semantically different, I would use another variable instead. It won't do any difference for the compiler, but it will be easier to read if variable do not change role in the middle.

Sridhar Iyer · Accepted Answer · 2010-05-25 21:11:08Z

0

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:

Skips the first argument.
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");
    }

answered May 25, 2010 at 21:11

Sridhar Iyer

2,8501 gold badge22 silver badges28 bronze badges

Comments

Jim Tshr · Accepted Answer · 2010-05-25 22:31:50Z

0

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

answered May 25, 2010 at 22:31

Jim Tshr

3571 gold badge3 silver badges10 bronze badges

Collectives™ on Stack Overflow

Allocating memory for a array to char pointer

7 Answers 7

Comments

1 Comment

Comments

Comments

1 Comment

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

7 Answers 7

Comments

1 Comment

Comments

Comments

1 Comment

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related