Creating a shell in C, execv() is unable to execute most commands

Question

So far the only commands I know to work in my shell are ls and echo. When I try something like touch or help, I either get nothing or a seg fault.

Here's my method to execute a command:

void mySystem(char * cmdargv[], char *searchpaths[], int numpaths, int numargs)
{
    int pid=fork();
    if(pid==0);
    {
        for(int i=0; i<numpaths; i++)
        {
            char * fullcmd = malloc(strlen(searchpaths[i] + strlen(cmdargv[0]) + 2));
            sprintf(fullcmd, "%s/%s", searchpaths[i], cmdargv[0]);
            //execv(fullcmd, cmdargv);
            printf("%s",fullcmd);
            free(fullcmd);

        }
        exit(0);
    }
    wait(NULL);
}

And here's my main program:

int main()
{
    const char space[]="\n"; //"deliminator," specifies where to tokenize commands

int numpaths;
int numargs;
char *searchpaths[MAX_PATHS];
char *fullCMD[MAX_CMD];
InitializeArray(fullCMD, MAX_CMD);
InitializeArray(searchpaths, MAX_PATHS);

char cmd[MAX_CMD];

SaveSearchPaths(searchpaths, (int *) &numpaths); //fills the searchpaths array with
                                                 //the system PATH and returns the
                                                 //the size of the array, numpaths

while(1)
{
    printf("> ");

    fgets(cmd, MAX_CMD, stdin);

    char *token;
    token=strtok(cmd, space);

    if(strncmp(cmd,"quit",4) == 0)
    {
        exit(0);
    }
    else if(strncmp(cmd, "path", 4)==0)
    {
        PrintAll(searchpaths, MAX_PATHS);
    }
    else
    {
        int i=0;
        while(token!=NULL)
        {
            InsertStringAt(fullCMD,token,i);
            token=strtok(NULL,space);
            i++;
            numargs+=1;
        }
        mySystem(fullCMD, searchpaths, numpaths, numargs);
    }

The InitializeArray() function initializes all of the indices in the array to NULL. InsertStringAt() inserts a string into a certain index in the array, and SaveSearchPaths() fills the searchpaths[] array with the tokenized System PATH.

Here's what happens when I type ls into the shell:

./a.out
> ls
a.out arrayOfStrings.c  arrayOfStrings.h  arrayOfStrings.h.gch  arrayOfStrings.o  arrayOfStringsTester.c  arrayOfStringsTester.o  myshell.c  mySystem.c  out  SaveSearchPaths.c

And here's what happens when I do help:

$ ./a.out
> help
$

Nothing happens.

And here's what happens when I do touch:

> touch file
*** Error in `./a.out': free(): invalid next size (fast): 0x00000000006a2130 ***
*** Error in `./a.out': free(): invalid next size (fast): 0x00000000006a2130 ***
Aborted (core dumped)

Any idea why only ls seems to work?

Answer 1

You have a typo here

strlen(searchpaths[i] + strlen(cmdargv[0]) + 2)
/*                   ^ the ) goes here  not   ^ here

this is valid because you are incrementing the searchpaths[i] pointer and executing strlen() over the resulting pointer which might cause undefined behavior, but is valid code and the compiler should compile it correctly.

The correct way is

strlen(searchpaths[i]) + strlen(cmdargv[0]) + 2

Also, you should check that malloc() did not return NULL which would indicate an error.

You could prevent this, and handle more errors if you store the result of strlen(). It's also good IMHO to get used to it because sometimes you might need the value more than once, and since strlen() computes the value it's more efficient to store it in those situations, in this situations the only benefit will be clarity and improving the ability to handle errors and maintain the code.

Answer 2

Not tested, I hope this works.

void mySystem(char * cmdargv[], char *searchpaths[], int numpaths, int numargs)
{
    int pid=fork();
    if(pid==0) /* remove junk semicolon */
    {
        for(int i=0; i<numpaths; i++)
        {
            char * fullcmd = malloc(strlen(searchpaths[i]) + strlen(cmdargv[0]) + 2); /* improve this expression */
            sprintf(fullcmd, "%s/%s", searchpaths[i], cmdargv[0]);
            //execv(fullcmd, cmdargv);
            printf("%s",fullcmd);
            free(fullcmd);

        }
        exit(0);
    }
    wait(NULL);
}