0

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?

chrk
  • 4,037
  • 2
  • 39
  • 47
Joseph
  • 11
  • 1
  • 3

4 Answers4

1

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;
}
John Bollinger
  • 160,171
  • 8
  • 81
  • 157
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
Jongware
  • 22,200
  • 8
  • 54
  • 100
user3629249
  • 16,402
  • 1
  • 16
  • 17
  • Thank you very much! Indeed the problem was at 'waitpid()' and I also didn't notice I put the free outside of the while loop. Whoops. By the way, I do use something, but in the full code, I just forgot to remove it here as it is irrelevant. Thanks again. – Joseph Nov 27 '14 at 07:20
  • `execvp`, like most other functions returns `-1` on failure. It never returns `0`. So the `execvp failed` code can never be executed. – Roland Illig Nov 27 '14 at 22:07
0

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);
chrk
  • 4,037
  • 2
  • 39
  • 47
0

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.

Roland Illig
  • 40,703
  • 10
  • 88
  • 121
  • The man page for `waitpid()` will also tell you which headers you should include. – John Bollinger Nov 26 '14 at 22:12
  • … on your specific system and operating system. If you want to develop software that runs not only on Linux, you should rather stick to the Open Group specifications than to the Linux man pages. – Roland Illig Nov 27 '14 at 22:01