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]; i++) {
char c = buf[i];

/* 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]

Re: Trouble resizing a pointer of pointers

benjamin sago wrote:[color=blue]
> Hello all. I am writing a command shell, but am having trouble with
> memory allocations.[/color]

Yes, you are. I think you need to re-read the relevant section of your
text book.
[color=blue]
> 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:[/color]

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

#include <stdlib.h> /* Required for malloc and friends */
[color=blue]
> char buf[] = "abcde fghij klmno"; /* sample string */
> int arg = 0, pos = 0, i, in_string = 0;
>
> char **args = NULL;
> args = malloc(1 + sizeof(char *));[/color]

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.
[color=blue]
> if (args == NULL) { printf("malloc failed"); exit(1); }[/color]

1 is not a portable value for exit. Use exit(EXIT_FAILU RE) instead.
[color=blue]
> for (i = 0; buf[i]; i++) {
> char c = buf[i];
>
> /* Space -> move on to the next argument */
> if (c == ' ' && pos != 0 && in_string == 0) {
> arg++; /* next argument */
> args[arg] = (char *) realloc(args, pos + 1);[/color]

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.
[color=blue]
> args[arg][0] = '\0';
> pos = 0;
> args = (char **) realloc(args, arg * sizeof(char *));
> /* add another line */[/color]

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, living in interesting times.
Web site - http://home.flash-gordon.me.uk/
comp.lang.c posting guidelines and intro:

http://clc-wiki.net/wiki/Intro_to_clc

[Vladimir S. Oka]

Re: Trouble resizing a pointer of pointers

Flash Gordon opined:
[color=blue]
> benjamin sago wrote:[color=green]
>> Hello all. I am writing a command shell, but am having trouble with
>> memory allocations.[/color]
>
> Yes, you are. I think you need to re-read the relevant section of
> your text book.
>[color=green]
> > 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:[/color]
>
> Is this your complete code, copied and pasted from your source file
> not re-typed? If so it has major problems.
>
> #include <stdlib.h> /* Required for malloc and friends */
>[color=green]
>> char buf[] = "abcde fghij klmno"; /* sample string */
>> int arg = 0, pos = 0, i, in_string = 0;
>>
>> char **args = NULL;
>> args = malloc(1 + sizeof(char *));[/color]
>
> 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.[/color]

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").
[color=blue]
>[color=green]
>> if (args == NULL) { printf("malloc failed"); exit(1); }[/color]
>
> 1 is not a portable value for exit. Use exit(EXIT_FAILU RE) instead.
>[color=green]
>> for (i = 0; buf[i]; i++) {
>> char c = buf[i];
>>
>> /* Space -> move on to the next argument */
>> if (c == ' ' && pos != 0 && in_string == 0) {
>> arg++; /* next argument */
>> args[arg] = (char *) realloc(args, pos + 1);[/color]
>
> 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.
>[color=green]
>> args[arg][0] = '\0';
>> pos = 0;
>> args = (char **) realloc(args, arg * sizeof(char
>> *));
>> /* add another line */[/color]
>
> 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.[/color]

--
"It's God. No, not Richard Stallman, or Linus Torvalds, but God."
(By Matt Welsh)

<http://clc-wiki.net/wiki/Introduction_to _comp.lang.c>

[Flash Gordon]

Re: Trouble resizing a pointer of pointers

Vladimir S. Oka wrote:[color=blue]
> Flash Gordon opined:
>[color=green]
>> benjamin sago wrote:[color=darkred]
>>> Hello all. I am writing a command shell, but am having trouble with
>>> memory allocations.[/color]
>> Yes, you are. I think you need to re-read the relevant section of
>> your text book.
>>[color=darkred]
>> > 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:[/color]
>> Is this your complete code, copied and pasted from your source file
>> not re-typed? If so it has major problems.
>>
>> #include <stdlib.h> /* Required for malloc and friends */
>>[color=darkred]
>>> char buf[] = "abcde fghij klmno"; /* sample string */
>>> int arg = 0, pos = 0, i, in_string = 0;
>>>
>>> char **args = NULL;
>>> args = malloc(1 + sizeof(char *));[/color]
>> 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.[/color]
>
> 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").[/color]

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

<snip>
--
Flash Gordon, living in interesting times.
Web site - http://home.flash-gordon.me.uk/
comp.lang.c posting guidelines and intro:

http://clc-wiki.net/wiki/Intro_to_clc