Why does getline() get the preceding line?

Question

bdoetsch@Kaladin:~/Documents/School$ ./shell
Shell(pid = 6955) 1> ls
command: ls
argv[i] = ls
argv[i] = ./shell
Parent says 'child process has been forked with pid=6956'
./shell
Parent says 'wait() returned so the child with pid=-1 is finished'
Shell(pid = 6955) 2>

This is homework I am working on, but I am a little stumped.

Hi, I am trying to write a shell program and I don't understand why getline is getting the preceding line of input.

plus the program will execute 'ls -al, 'pwd and a few others. but not just ls.

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

int tokenizer(char *str,char **argv);
void doCommand(char **argv);

int main()
{
    pid_t pid;
    char *command;
    int Num_bytes_read;
    size_t nbytes = 60;
    int NumCommand = 0;
    char **argv;

    int NumOfArgs;

    command = (char *) malloc (nbytes + 1);
    command = NULL;
    while(1)
    {   
        NumCommand++;
        printf("Shell(pid = %d) %d> ",getpid(),NumCommand);

        fflush(stdin);

        Num_bytes_read = getline(&command,&nbytes,stdin);

        printf("command: %s",command);        
        if(Num_bytes_read == -1)
        {
            printf("\n ERRor\n");
        }
        else
        {  
            int x = tokenizer(command,argv);
            int i;        

            for (i = 0; i < (x+1); ++i)
                printf ("argv[i] = %s\n",argv[i]);
            //if (strcmp(argv[0], "quit")){
            //break;
            //}
            doCommand(argv);
        }
    }
    return 0;
}

int tokenizer(char *str, char **argv)
{
    //const char s[2] = " "; 
    char ** res  = NULL;
    char * p    = strtok (str, " \n");
    int n_spaces = 0, i;
    while (p) 
    {
        res = realloc (res, sizeof (char*) * ++n_spaces);
        if (res == NULL)
            exit (-1); /* memory allocation failed */
        res[n_spaces-1] = p;
        argv[n_spaces-1]= res[n_spaces-1];

        p = strtok (NULL, " \n");
    }
    // realloc one extra element for the last NULL 

    res = realloc (res, sizeof (char*) * (n_spaces+1));
    res[n_spaces] = 0;

    argv = res;
    return n_spaces;
}      

void doCommand(char **argv)
{
    pid_t  pid;
    pid_t cpid; /* Pid of child to be returned by wait. */
    int fd[2];  // dual pipeline
    int status; /* Exit status of child. */

    int nbytes;
    int commandStatus;

    pipe(fd);

    pid = fork();   // Preceding with fork]
    if (pid < 0) 
    {   
        printf("forking child process failed\n");
        exit(1);
    }
    else if (pid == 0)     // fork for the child
    {    
        close(fd[0]); // close up reader side of pipe

        cpid = getpid();  

        /* Send "string" through the output side of pipe */  
        write(fd[1], &cpid,sizeof(cpid));
        //argv[0] = "ls";
        //argv[1] = NULL;
        commandStatus = execvp(*argv, argv);
        if (commandStatus < 0)  /* execute the command  */
        {    
            printf("Try again, command failed\n");
            exit(1);
        }
    }
    else if (pid > 1)                // fork for the parent
    {  
        close(fd[1]);

        /* Read in the child pid from the pipe */
        nbytes = read(fd[0], &cpid , sizeof(cpid));
        printf("Parent says 'child process has been forked with pid=%ld'\n",(long)cpid);
        wait(NULL);
        cpid = wait(&status);      /* wait for completion  */

        printf("Parent says 'wait() returned so the child with pid=%ld is finished'\n",(long)cpid);
    }
}

`fflush()` is not defined for input stream (or input/output stream where the last operation was input). — pmg, Feb 09 '15 at 13:30
`argv` is never initialized and there is a access `argv[n_spaces-1]= res[n_spaces-1];` leading to UB — Gopi, Feb 09 '15 at 13:31
[don't cast malloc](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) — Iharob Al Asimi, Feb 09 '15 at 13:35
Error messages belong on stderr and should be useful. For example: `if(pid < 0) { perror("fork"); exit(EXIT_FAILURE);}` — William Pursell, Feb 09 '15 at 13:52

Iharob Al Asimi · Accepted Answer · 2015-02-09T13:55:32.007

The problem is that argv is not really initialized, because you are altering the local pointer in tokenizer(), instead of the pointer in main().

What you have to do is pass argv's address instead of the pointer itself, and then change tokenizer() like this

int tokenizer(char *str, char ***argv)
{
    char ** res      = NULL;
    char *  p        = NULL;
    int     n_spaces = 0;

    if ((str == NULL) || (argv == NULL))
        return 0;

    p     = strtok (str, " \n"); 
    *argv = NULL;
    while (p)
    {
        res = realloc (*argv, sizeof(char *) * ++n_spaces);
        if (res == NULL)
        {
            free(*argv);
            return 0;
        }
        res[n_spaces - 1] = p;
        *argv             = res;
        p                 = strtok (NULL, " \n");
    }
    res = realloc (*argv, sizeof(char *) * (n_spaces + 1));
    if (res == NULL)
    {
        free(*argv);
        return 0;
    }
    res[n_spaces] = 0;
    *argv         = res;

    return n_spaces;
}

and in main

int x = tokenizer(command, &argv);
/*                         ^ address of argv */

you never checked the last realloc too, so I fixed some of other things I believe you was doing a little bit unsafe.

exit() on malloc/realloc failure is not really necessary in your case, you can just free() the already alloced pointer and just return 0, then handle that in main().

Also, I recommend the usage of the const qualifier wherever it makes sense, for example

void doCommand(const char *const *argv)

And also, don't fflush() stdin it's undefined behavior.

Iharob, what program did you use to format my code so nicely? — Brian Doetsch, Feb 09 '15 at 13:44
shell.c: In function ‘main’: shell.c:45:32: warning: passing argument 2 of ‘tokenizer’ from incompatible pointer type int x = tokenizer(command,&argv); ^ shell.c:8:6: note: expected ‘char **’ but argument is of type ‘char ***’ int tokenizer(char *str,char **argv); ^ shell.c: At top level: shell.c:64:6: error: conflicting types for ‘tokenizer’ int tokenizer(char *str, char ***argv) ^ shell.c:8:6: note: previous declaration of ‘tokenizer’ was here int tokenizer(char *str,char **argv); — Brian Doetsch, Feb 09 '15 at 13:58
I was guessing my error was in way I allocating memory.. so, *** is an array of pointers to strings. wicked. thank you — Brian Doetsch, Feb 09 '15 at 14:06

Why does getline() get the preceding line?

1 Answers1