Fill char*[] from strtok

Question

I have problems getting following Code to work. It parses a users input into a char*[] and returns it. However the char* command[] does not accept any values and stays filled with NULL... whats going on here?

void* setCommands(int length){         
    char copy[strlen(commandline)];    //commandline is a char* read with gets();
    strcpy(copy, commandline);
    char* commands[length];
    for (int x=0; x<length; x++)
        commands[x] = "\0";
    int i = 0;
    char* temp;
    temp = strtok (copy, " \t");
    while (temp != NULL){
        commands[i] = temp;    //doesnt work here.. commands still filled with NULL afterwards
        i++;
        printf("word:%s\n", temp);
        temp = strtok (NULL, " \t");
    }   
    commands[i] = NULL;
    for (int u=0; u<length; u++)
        printf("%s ", commands[i]);
    printf("\n");
    return *commands;
}

You may assume, that commandline != NULL, length != 0

`char* commands[length];` and `commands[x] = "\0";` - what are you trying to do here? If you want to write to a pointer, then first allocate memory for it using `malloc`. At the moment your declaration says 'declare commands as array of length equal to 'length' of pointer to char'. Or just declare a 2D char array so you don't bother with manual memory management. — Nobilis, Jun 11 '13 at 02:14
The for-loop was a try to get the array's elements initialized, as i thought that was the problem... well it wasnt. — Torhan Bartel, Jun 11 '13 at 02:17

paxdiablo · Accepted Answer · 2013-06-11T02:35:34.983

0

commands[i] = NULL;
for (int u=0; u<length; u++)
    printf("%s ", commands[i]);

Take a very good look at that code. It uses u as the loop control variable but prints out the element based on i.

Hence, due to the fact you've set commands[i] to NULL in the line before the loop, you'll just get a series of NULLs.

Use commands[u] in the loop rather than commands[i].

In addition to that:

void* setCommands(int length){         
    char* commands[length];
    :
    return *commands;
}

will only return one pointer, the one to the first token, not the one to the array of token pointers. You cannot return addresses of local variables that are going out of scope (well, you can, but it may not work).

And, in any case, since that one pointer most likely points to yet another local variable (somewhere inside copy), it's also invalid.

If you want to pass back blocks of memory from functions, you'll need to look into using malloc, in this case both for the array of pointers and the strings themselves.

edited Jun 11 '13 at 02:35

answered Jun 11 '13 at 02:15

paxdiablo

854,327
234
1,573
1,953

The for-loop was a try to get the array's elements initialized, as i thought that was the problem... well it wasnt. – Torhan Bartel Jun 11 '13 at 02:18
What? Your comment about changing `temp` and making all values point to one final value is incorrect. – paddy Jun 11 '13 at 02:19
i tried commands[i] = strdup (temp); however it doesnt fix the problems... still NULLs everywhere – Torhan Bartel Jun 11 '13 at 02:21
@Torhan (and paddy), see the update. It was actually a totally _different_ issue from what I originally thought. – paxdiablo Jun 11 '13 at 02:31
@TorhanBartel, that's not your only problem, so no, _not_ as easy as you think :-) See the update, I think you'll be back shortly with another question. – paxdiablo Jun 11 '13 at 02:37
Which explains the "Invalid read of Size 1" error i got now :D... Ok.. so first i need to malloc the Array itself, then every pointer inside it? – Torhan Bartel Jun 11 '13 at 02:44
@TorhanBartel: malloc the array but you can just use strdup for each token. – paxdiablo Jun 11 '13 at 02:46
Ok... for the array length*sizeof(char*) should be enough, shouldnt it? – Torhan Bartel Jun 11 '13 at 02:48
Yes, but remember it's a `char**` now. `char **commands = malloc (length * sizeof(*commands));` ... `commands[i] = strdup (temp);`. I prefer the `*commands` to `char*` since it's only _one_ thing you need to change if the underlying type changes. – paxdiablo Jun 11 '13 at 02:50

score 0 · Answer 2 · answered Jun 11 '13 at 02:50

You have a number of issues... Your program will be exhibiting undefined behaviour currently, so until you address the issues you cannot hope to predict what's going on. Let's begin.

The following string is one character too short. You forgot to include a character for the string terminator ('\0'). This will lead to a buffer overrun during tokenising, which might be partly responsible for the behaviour you are seeing.

char copy[strlen(commandline)];   // You need to add 1
strcpy(copy, commandline);

The next part is your return value, but it's a temporary (local array). You are not allowed to return this. You should allocate it instead.

// Don't do this:
char* commands[length];
for (int x=0; x<length; x++)
    commands[x] = "\0";         // And this is not the right way to zero a pointer

// Do this instead (calloc will zero your elements):
char ** commands = calloc( length, sizeof(char*) );

It's possible for the tokenising loop to overrun your buffer because you never check for length, so you should add in a test:

while( temp != NULL && i < length )

And because of the above, you can't just blindly set commands[i] to NULL after the loop. Either test i < length or just don't set it (you zeroed the array beforehand anyway).

Now let's deal with the return value. Currently you have this:

return *commands;

That returns a pointer to the first token in your temporary string (copy). Firstly, it looks like you actually intended to return an array of tokens, not just the first token. Secondly, you can't return a temporary string. So, I think you meant this:

return commands;

Now, to deal with those strings... There's an easy way, and a clever way. The easy way has already been suggested: you call strdup on each token before shoving them in memory. The annoying part of this is that when you clean up that memory, you have to go through the array and free each individual token.

Instead, let's do it all in one hit, by allocating the array AND the string storage in one call:

char **commands = malloc( length * sizeof(char*) + strlen(commandline) + 1 );
char *copy = (char*)(commands + length);
strcpy( copy, commandline );

The only thing I didn't do above is zero the array. You can do this after the tokenising loop, by just zeroing the remaining values:

while( i < length ) commands[i++] = NULL;

Now, when you return commands, you return an array of tokens which also contains its own token storage. To free the array and all strings it contains, you just do this:

free( commands );

Putting it all together:

void* setCommands(int length)
{
    // Create array and string storage in one memory block.
    char **commands = malloc( length * sizeof(char*) + strlen(commandline) + 1 );
    if( commands == NULL ) return NULL;
    char *copy = (char*)(commands + length);
    strcpy( copy, commandline );

    // Tokenise commands
    int i = 0;
    char *temp = strtok(copy, " \t");

    while( temp != NULL && i < length )
    {
        commands[i++] = temp;
        temp = strtok(NULL, " \t");
    }

    // Zero any unused tokens
    while( i < length ) commands[i++] = NULL;

    return commands;
}

Fill char*[] from strtok

2 Answers2