waitpid() not waiting for child

Question

I wrote a really basic shell and for some reason, when I use fork() and then waitpid() the parent process won't wait for the child.

#include <stdio.h>
#include <unistd.h>
#include <string.h>
#include <stdlib.h>
#include <sys/syscall.h>
#include <linux/limits.h>
#include "LineParser.h"
#include <termios.h>

#define MAX_STR 2048
void execute(cmdLine *pCmdLine);


int main()
{
    char isContinuing = 1;
    char path[PATH_MAX];
    char str[MAX_STR];
    char something[MAX_STR+PATH_MAX];
    cmdLine* cmd;
    while(isContinuing)
    {
        getcwd(path, PATH_MAX);
        printf("%s$ ", path);
        fgets(str, MAX_STR, stdin);
        if(!strncmp(str, "quit", strlen("quit")))
        {
            isContinuing = 0;
        }
        else
        {
            cmd = parseCmdLines(str);
            if(cmd->arguments != '\0')
            {
                execute(cmd);
            }
        }
    }

    freeCmdLines(cmd);
    return 0;
}

void execute(cmdLine *pCmdLine)
{
    pid_t id = fork();

    if(id == 0)
    {
        printf("I AM CHILD.\n");
        if(!execvp(pCmdLine->arguments[0], pCmdLine->arguments))
        {
            perror("execvp failed.\n");
            exit(1);
        }
        exit(0);
    }
    printf("I AM PARENT.\n");
    printf("WAITING FOR CHILD.\n");
    waitpid(id);
    printf("DONE WAITING\n");

}

LineParser header file is mine and it is fully working. Now, for some reason, only the first command is working as expected, let's assume an input "echo hi", the output is:

I AM PARENT.
WAITING FOR CHILD.
I AM CHILD.
DONE WAITING.

as expected and then it prints "hi" and the path, waiting for a command again. For some reason, when I enter the SAME input "echo hi" the second time, the output is:

I AM PARENT.
WAITING FOR CHILD.
DONE WAITING.
$PATH$ //(WITHOUT WAITING FOR INPUT !!!)
I AM CHILD.
hi
//and here waiting for input//

Why does this happen?

Answer 1

There are several problems with your code:

1. not clearing malloc'd memory on every iteration through the while loop
2. putting a exit() statement in unreachable code
3. incorrect parameter list for the waitpid() function
4. unclear delination between parent code and child code in execute function
5. unused variable something
6. failed to check return value from fgets function
7. missing #include for sys/types.h
8. missing #include for sys/wait.h
9. IMO: the question should have included the definition of struct cmdLine

So here is a compilable version of your code. The compiler found many problems with the original code.

#include <stdio.h>
#include <unistd.h>
#include <string.h>
#include <stdlib.h>
#include <sys/syscall.h>
#include <linux/limits.h>
//#include "LineParser.h"
#include <termios.h>
#include <sys/types.h>
#include <sys/wait.h> // prototype for waitpid()


//note: pid_t waitpid(pid_t pid, int *status, int options);


struct cmdLine
{
    char ** arguments; // arguments[x] = ptr to an argument string
};

#define MAX_STR  (2048)
#define MAX_PATH (256)
void execute(struct cmdLine *);
struct cmdLine * parseCmdLines( char * );
void freeCmdLines( struct cmdLine * );



int main()
{
    char path[PATH_MAX];
    char str[MAX_STR];
    //char something[MAX_STR+PATH_MAX];
    struct cmdLine* pCmd = NULL;

    while(1)
    {
        getcwd(path, PATH_MAX);
        printf("%s$ ", path);
        if( NULL == fgets(str, MAX_STR, stdin) )
        {
            perror( "fgets failed" );
            exit( EXIT_FAILURE );
        }

        // implied else

        if(!strncmp(str, "quit", strlen("quit")))
        { // then strings equal
            break;  // exit while loop (and pgm)
        }

        // implied else input not equal 'quit'

        pCmd = parseCmdLines(str);
        if( (NULL != pCmd) && (NULL != pCmd->arguments) )
        { // then one or more arguments entered/parsed
            execute(pCmd);
        } // end if

        freeCmdLines(pCmd);  // free all strings memory, then free struct memory
        pCmd = NULL; // cleanup
    } // end while

    return 0;
} // end function: main


void execute(struct cmdLine *pCmdLine)
{
    int status = 0;
    pid_t id = fork();

    if(id == 0)
    { // then, child
        printf("I AM CHILD.\n");
        if(!execvp(pCmdLine->arguments[0], pCmdLine->arguments))
        { // if no error then never gets here
            perror("execvp failed.\n");
        } // end if
    }

    else
    { // else, parent
        printf("I AM PARENT.\n");
        printf("WAITING FOR CHILD.\n");
        waitpid(id, &status, 0);
        printf("DONE WAITING\n");
    } // end if
} // end function: execute

Answer 2

Your main problem is that you don’t let the compiler check your code. You should generally enable the compiler warnings and try to understand them.

$ gcc -Wall -Wextra -Werror -Os -c myshell.c

This is the minimum command line I use. When your code compiles with these settings, you have already eliminated a bunch of hard-to-find bugs in your code. Among these bugs is, as others already have mentioned, the call to waitpid.

Have a look at http://pubs.opengroup.org/onlinepubs/7908799/xsh/waitpid.html. The Open Group specification requires that you #include the two headers <sys/types.h> and <sys/wait.h> before using the waitpid function. Your program doesn’t do this.

Answer 3

You invoke undefined behavior by calling the waitpid() function with the wrong number of arguments. Anything could happen.

This simplified variant of your code works fine for me:

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

int main ()
{
    int i;

    for (i = 0; i < 3; i += 1)
    {
        pid_t id = fork();

        if(id == 0)
        {
            char *argv[] = { "echo", "hi", NULL };

            printf("I AM CHILD.\n");
            execvp("echo", argv);
            /* failed to exec */
            perror("execvp failed.\n");
            exit(1);
        } else if (id < 0) {
            perror("fork failed.\n");
            exit(1);
        }
        printf("I AM PARENT.\n");
        printf("WAITING FOR CHILD.\n");
        waitpid(id, NULL, 0);
        printf("DONE WAITING\n");
    }

    return 0;
}

Answer 4

Your call to waitpid(2) is wrong.

According to man 2 waitpid, it's:

pid_t waitpid(pid_t pid, int *status, int options);

You probably need to define an int and call it as:

waitpid(id, &status, 0);

or use the simpler version wait(2), which will work for any child:

wait(&status);