CODEWITHSUNDEEP
cforkexecve
SolRac
SolRac

Reputation: 33

Fork and execve segmentation fault

My program should use fork and exec system calls. The exec should change the child process such that it takes another command as an argument and executes that command. For example, to display the message of the day:

 ./myexec cat /etc/motd

This is my current code 

extern char **environ;      /* environment info */
main(int argc, char **argv) {
     /* argc -- number of arguments */
     /* argv -- an array of strings */

    char *argvNew[argc + 1];
    int pid;

    for(int i=0; i<argc; i++){
        argvNew[i] = argv[i];
    }
    argvNew[argc + 1] = NULL;
    printf("After For: %s\n",argvNew[0]);
    printf("After For: %s\n",argvNew[1]);
    printf("After For: %s\n",argvNew[2]);
    printf("After For: %s\n",argvNew[3]);


    if ((pid = fork()) < 0) {
        fprintf(stderr, "Fork error%sstrerror\n", strerror(errno));

        exit(1);
    }
    else if (pid == 0) {
        /* child process */
        if (execve(argvNew[0], argvNew, environ) < 0) {
            fprintf(stderr, "Execve error%d %s\n",errno,strerror(errno));
            exit(1);
        }
    }
    else {
        /* parent */
    wait(0);        /* wait for the child to finish */
    }

}

After running ./myexecv cat etc/motd nothing happens; just the print statements. Any advice going forward?

Upvotes: 3

Views: 3657

Answers (2)

Christophe
Christophe

Reputation: 73376

The calling parameters of execve() require the first parameter to be the filename to execute. Unfortunately you pass it argvNew[0] which is the same value as argv[0]. This means that you call and call again your own program and never the script. You need to shift the parameters by one:

...
for(int i=1; i<argc; i++){
    argvNew[i-1] = argv[i];
}
argvNew[argc-1] = NULL;
...

Upvotes: 1

Sam Varshavchik
Sam Varshavchik

Reputation: 118340

There are multiple bugs in the shown code.

    for(int i=0; i<argc; i++){
            argvNew[i] = argv[i];
    }
    argvNew[argc+1] = NULL;

On it's face value, the NULL assignment is wrong, and will result in undefined behavior because argvNew is declared as

    char *argvNew[argc + 1];

So the array contains values argvNew[0] through argvNew[argc], and argvNew[argc+1]=NULL; runs off past the end of the array, that results in undefined behavior. This should obviously be

    argvNew[argc] = NULL;

But even that would also be wrong, because:

 execve(argvNew[0], argvNew, environ);

argvNew[0] is copied from argv[0], which is the name of this program being executed. This is going to fork and run the same program, in the child process.

You will end up forkbombing yourself. If this is a shared server, you will make the system administrator very very mad.

You need to remove argv[0] from the equation, and copy over only argv[1], and on. The correct loop, and copy, is:

    int pid;
    char *argvNew[argc];

    for(int i=1; i<argc; i++){
            argvNew[i-1] = argv[i];
    }
    argvNew[argc-1] = NULL;

Upvotes: 2

Related Questions