Bad address error with execvp

Question

I'm trying to make a shell "bosh>" which takes in Unix commands and keep getting a bad address error. I know my code reads in the commands and parses them but for some reason, I cannot get them to execute, instead, I get a "bad address" error.

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/wait.h>
#include <sys/types.h>
#include <string.h>
#include <sys/wait.h>
#define MAX_LINE 128
#define MAX_ARGS 10



int main(){
    pid_t pid;
    char command[MAX_LINE]; /*command line buffer*/
    char *commandArgs[MAX_ARGS]; /*command line arg*/
    int i;
    char *sPtr=strtok(command," ");
    int n=0;


    printf("bosh>");
    fgets(command, MAX_LINE-1,stdin);
    command[strlen(command)-1]='\0';


    while(strcmp(command,"quit")!=0)
    {
        n=0;

        sPtr=strtok(command," ");
        while(sPtr&&n<MAX_ARGS)
        {
            sPtr=strtok(NULL," ");
            n++;
        }

        commandArgs[0]=malloc(strlen(command)+1);
        strcpy(commandArgs[0],command);

        if(fork()==0)
        {
            execvp(commandArgs[0],commandArgs);
            perror("execvp failed");
            exit(2);
        }
        pid=wait(NULL);


        printf("%s",">" );
        fgets(command, MAX_LINE-1,stdin);
        command[strlen(command)-1]='\0';
    }

    printf("Command (%d) done\n", pid);

    return 0;


}

First of all, nice dogs in picture. A terminating null byte ( '\0' ) is stored after the last character in the buffer read by `fgets`. So you don't have to put a `'\0'`. — Tony Tannous, Mar 01 '17 at 20:29
@TonyTannous that is to strip the newline... (though it is incorrect as well). — Antti Haapala -- Слава Україні, Mar 01 '17 at 20:31
you'd want to check that the last character indeed is a `'\n'` before chopping it off. — Antti Haapala -- Слава Україні, Mar 01 '17 at 20:32
I am not sure but I think there's an issue with the way you are putting your arguments in the commandArgs. You are only storing the first argument. And the final argument has to be NULL. — Tony Tannous, Mar 01 '17 at 20:33

score 3 · Answer 1 · edited May 23 '17 at 10:29

These two lines are the culprit:

commandArgs[0]=malloc(strlen(command)+1);
strcpy(commandArgs[0],command);

First of all, malloc(strlen(...)) followed by strcpy is what the POSIX function strdup already does. But then, you don't need to even copy the string - it is enough to just store the pointer to the original string into commandArgs[0]:

commandArgs[0] = command;

But then, how does execvp how many arguments the command is going to take? If you read the manuals carefully, they'd say something like:

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.

Your argument array is not NULL-terminated. To fix it, use

commandArgs[0] = command;
commandArgs[1] = NULL; // !!!!

(Then you'd notice that you'd actually want to assign the arguments within the strtok parsing loop, so that you can actually assign all of the arguments into the commandArgs array; and compile with all warnings enabled and address those, and so forth).

score 1 · Answer 2 · answered Mar 01 '17 at 20:32

You initialize sPtr in its declaration, which you do not need to do because you never use the initial value. But the initialization produces undefined behavior because it depends on the contents of the command array, which at that point are indeterminate.

The array passed as the second argument to execvp() is expected to contain a NULL pointer after the last argument. You do not ensure that yours does.

You furthermore appear to drop all arguments to the input command by failing to assign tokens to commandArgs[]. After tokenizing you do copy the first token (only) and assign the copy to the first element of commandArgs, but any other tokens are ignored.

I've pointed that out in comments. There's a problem with argument initialization. Only first argument probably getting assigned and there's no NULL after the last argument. +1 — Tony Tannous, Mar 01 '17 at 20:35

Bad address error with execvp

2 Answers2