CODEWITHSUNDEEP
cfgets
unicornication
unicornication

Reputation: 627

Storing as using result of strtok in array in C

I'm trying to split the input from fgets using strtok, and store the results in an array, i.e. newArgs, so I can then call execvp and essentially execute the input passed by fgets.

E.g. ls -la will map to /bin/ls -la and execute correctly.

#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
#include <string.h>
#include <stdlib.h>
#include <stdio.h>

int main(int argc, char * argv[])
{
    char buff[1024];
    fgets(buff, 1024, stdin);
    buff[strcspn(buff, "\n")] = 0;
    printf("%s\n", buff);
    printf("%d\n", strlen(buff));
    char *newArgs[30];
    char *token;
    char delim[2] = " ";
    token = strtok(buff, delim);
    int i = 0;
    while(token != NULL) 
    {
        if(newArgs[i])
        {
            sprintf(newArgs[i], "%s", token);
            printf("%s\n", newArgs[i]); 
        }
        token = strtok(NULL, delim);
        i++;
    }
    execvp(newArgs[0], newArgs);

    return 0;
}

I keep getting a Segmentation fault, even though I'm checking the existence of newArgs[i], which is a little odd. Any ideas as to what's going wrong?

Upvotes: 2

Views: 1416

Answers (4)

rici
rici

Reputation: 241921

There is absolutely no reason to copy the tokens you are finding in buff.

That won't always be the case, but it certainly is here: buff is not modified before the execvp and execvp doesn't return. Knowing when not to copy a C string is not as useful as knowing how to copy a C string, but both are important.

Not copying the strings will simplify the code considerably. All you need to do is fill in the array of strings which you will pass to execvp:

 char* args[30]; /* Think about dynamic allocation instead */
 char** arg = &args[0];
 *arg = strtok(buff, " ");
 while (*arg++) {
   /* Should check for overflow of the args array */
   *arg = strtok(NULL, " ");
 }
 execvp(args[0], args);

Note that the above code will store the NULL returned by strtok at the end of the args array. This is required by execvp, which needs to know where the last arg is.

Upvotes: 0

R Sahu
R Sahu

Reputation: 206717

Problems I see:

  1. You are using uninitialized values of newArgs[i]. You have:

    char *newArgs[30];

    This is an array of uninitialized pointers. Then, you go on to use them as:

    if(newArgs[i])

    That is cause for undefined behavior. You can fix that by initializing the pointers to NULL.

    char *newArgs[30] = {};

  2. You haven't allocated memory for newArgs[i] before calling 

    sprintf(newArgs[i], "%s", token);

    That is also cause for undefined behavior. You can fix that by using:

    newArgs[i] = strdup(token);

  3. The list of arguments being passed to execvp must contains a NULL pointer.

    From http://linux.die.net/man/3/execvp (emphasis mine):

    The execv(), execvp(), and execvpe() functions provide an array of pointers to null-terminated strings that represent the argument list available to the new program. The first argument, by convention, should point to the filename associated with the file being executed. The array of pointers must be terminated by a NULL pointer.

    You are missing the last requirement. You need o make sure that one of the elements of newArgs is a NULL pointer. This problem will go away if you initialize the pointers to NULL.

Upvotes: 0

Rishikesh Raje
Rishikesh Raje

Reputation: 8614

You are not allocating memory for newArgs before storing it in the string. Add

    newArgs[i] = malloc(strlen(token));

before the if statement inside the for loop.

Upvotes: 0

user5886608
user5886608

Reputation: 46

You're not allocating any memory for each element of newArgs. Try using a multi-dimensional array, like newArgs[30][100]. Don't forget to ensure they're null terminated.

Upvotes: 1

Related Questions