Trouble resizing a pointer of pointers

[benjamin sago]

Hello all. I am writing a command shell, but am having trouble with
memory allocations. I am trying to tokenise a string into a **char
array for execvp (the unix launch-this-program function), but I get
segmentation faults when I run it.

Here is the code that I use to parse the string, which fails:

char buf[] = "abcde fghij klmno"; /* sample string */
int arg = 0, pos = 0, i, in_string = 0;

char **args = NULL;
args = malloc(1 + sizeof(char *));
if (args == NULL) { printf("malloc failed"); exit(1); }

for (i = 0; buf; i++) {
char c = buf;

/* Space -> move on to the next argument */
if (c == ' ' && pos != 0 && in_string == 0) {
arg++; /* next argument */
args[arg] = (char *) realloc(args, pos + 1);
args[arg][0] = '\0';
pos = 0;
args = (char **) realloc(args, arg * sizeof(char *));
/* add another line */
if (args == NULL) { printf("realloc failed (1)");
exit(1); }
continue;
}

/* Quotes -> toggle being in a string */
if (c == '"') {
in_string = !in_string;
continue;
}

/* backslash -> skip a character */
if (c == '\\') c = buf[++i];

/* Anything else -> add to the args list */
args[arg] = realloc(args, (pos) * sizeof(char) + 1); /* resize
to fit the new character */
if (args[arg] == NULL) { printf("realloc failed (2)"); exit(1);
}
args[arg][pos] = c;
args[arg][pos+1] = '\0'; /* so it is a null-terminated string
*/
pos++;
}

The length of **args should be increasing to fit whatever I try to put
in it, but it is not.

It parses the first letter fine, but segfaults on the second. Sometimes
it likes the second letter too, but segfaults on the third. I ran it
through the gnu debugger, and it always faults on the line
"args[arg][pos+1] = '\0';", so I am thinking that I am not allocating
enough, or allocating in the wrong place.

What is wrong?

 

[Flash Gordon]

Hello all. I am writing a command shell, but am having trouble with
memory allocations.

Yes, you are. I think you need to re-read the relevant section of your
text book.

> I am trying to tokenise a string into a **char
array for execvp (the unix launch-this-program function), but I get
segmentation faults when I run it.

Here is the code that I use to parse the string, which fails:

Is this your complete code, copied and pasted from your source file not
re-typed? If so it has major problems.

char buf[] = "abcde fghij klmno"; /* sample string */
int arg = 0, pos = 0, i, in_string = 0;

char **args = NULL;
args = malloc(1 + sizeof(char *));

Why initialise args then immediately overwrite the initial value?
char **args = malloc(1 + sizeof(char *));
Although I still don't think this is what you want. Why the 1+? What are
you trying to achieve?

The more normal form is:
char **args = malloc(N + sizeof *args);
where N is the number of elements you want space for.

if (args == NULL) { printf("malloc failed"); exit(1); }

1 is not a portable value for exit. Use exit(EXIT_FAILURE) instead.

for (i = 0; buf; i++) {
char c = buf;

/* Space -> move on to the next argument */
if (c == ' ' && pos != 0 && in_string == 0) {
arg++; /* next argument */
args[arg] = (char *) realloc(args, pos + 1);

What are you trying to do here? I really think this is nothing like what
you are thinking. malloc & realloc know nothing about 2D arrays, but I'm
guessing you think they do. In any case, this is definitely completely
wrong. I suggest you read the comp.lang.c FAQ starting with sections 6 &
7, but read the rest as well.
http://www.eskimo.com/~scs/c-faq.com/aryptr/index.html
http://www.eskimo.com/~scs/c-faq.com/malloc/index.html

Also, don't cast the return value of realloc. It isn't required.

Then you need to check the return value as well.

args[arg-1] = malloc(space required for string, not
forgetting null termination);

The -1 is because you have already incremented arg.

args[arg][0] = '\0';
pos = 0;
args = (char **) realloc(args, arg * sizeof(char *));
/* add another line */

A bit better, but more likely
args = realloc(args, arg * sizeof *args);

<snip>

I've not checked the rest because it is already so far off the mark by
here that I didn't see the point. I've also not checked the logic of
your argument passing.

 

[Vladimir S. Oka]

Flash Gordon opined:

Hello all. I am writing a command shell, but am having trouble with
memory allocations.

Yes, you are. I think you need to re-read the relevant section of
your text book.

I am trying to tokenise a string into a **char
array for execvp (the unix launch-this-program function), but I get
segmentation faults when I run it.

Here is the code that I use to parse the string, which fails:

Is this your complete code, copied and pasted from your source file
not re-typed? If so it has major problems.

char buf[] = "abcde fghij klmno"; /* sample string */
int arg = 0, pos = 0, i, in_string = 0;

char **args = NULL;
args = malloc(1 + sizeof(char *));

Why initialise args then immediately overwrite the initial value?
char **args = malloc(1 + sizeof(char *));
Although I still don't think this is what you want. Why the 1+? What
are you trying to achieve?

The more normal form is:
char **args = malloc(N + sizeof *args);
where N is the number of elements you want space for.

You mean:

char **args = malloc(N * sizeof *args);

or I have misread both the OP or your reply (I read it as "space for N
pointers to `char` is required").

if (args == NULL) { printf("malloc failed"); exit(1); }

1 is not a portable value for exit. Use exit(EXIT_FAILURE) instead.

for (i = 0; buf; i++) {
char c = buf;

/* Space -> move on to the next argument */
if (c == ' ' && pos != 0 && in_string == 0) {
arg++; /* next argument */
args[arg] = (char *) realloc(args, pos + 1);

What are you trying to do here? I really think this is nothing like
what you are thinking. malloc & realloc know nothing about 2D arrays,
but I'm guessing you think they do. In any case, this is definitely
completely wrong. I suggest you read the comp.lang.c FAQ starting
with sections 6 & 7, but read the rest as well.
http://www.eskimo.com/~scs/c-faq.com/aryptr/index.html
http://www.eskimo.com/~scs/c-faq.com/malloc/index.html

Also, don't cast the return value of realloc. It isn't required.

Then you need to check the return value as well.

args[arg-1] = malloc(space required for string,
not
forgetting null termination);

The -1 is because you have already incremented arg.

args[arg][0] = '\0';
pos = 0;
args = (char **) realloc(args, arg * sizeof(char
*));
/* add another line */

A bit better, but more likely
args = realloc(args, arg * sizeof *args);

<snip>

I've not checked the rest because it is already so far off the mark
by here that I didn't see the point. I've also not checked the logic
of your argument passing.

 

[Flash Gordon]

Flash Gordon opined:

Hello all. I am writing a command shell, but am having trouble with
memory allocations.

Yes, you are. I think you need to re-read the relevant section of
your text book.

I am trying to tokenise a string into a **char
array for execvp (the unix launch-this-program function), but I get
segmentation faults when I run it.

Here is the code that I use to parse the string, which fails:

Is this your complete code, copied and pasted from your source file
not re-typed? If so it has major problems.

char buf[] = "abcde fghij klmno"; /* sample string */
int arg = 0, pos = 0, i, in_string = 0;

char **args = NULL;
args = malloc(1 + sizeof(char *));

Why initialise args then immediately overwrite the initial value?
char **args = malloc(1 + sizeof(char *));
Although I still don't think this is what you want. Why the 1+? What
are you trying to achieve?

The more normal form is:
char **args = malloc(N + sizeof *args);
where N is the number of elements you want space for.

You mean:

char **args = malloc(N * sizeof *args);

or I have misread both the OP or your reply (I read it as "space for N
pointers to `char` is required").

You are correct, I did mean to multiply. I blame this notebooks keyboard ;-)

<snip>