3

I am creating a Linux type shell program for a school project. So far I have implemented the basic Linux external commands like "ls","ps", etc., using execvp and basic pipes. As part of the project, the user can either run the program in interactive mode or batch mode. In interactive mode the user just enters a command when prompted. For batch mode, the user specifies a file in the command line where there is a list of commands to execute.

The problem I am having is in batch mode. In batch mode, if an invalid command is listed (e.g. "kdfg", for which "kdfg: command not found" while be the output), everything afterward continues, but everything afterward is executed twice. so if I have a "kghd" on one line and the next line is "ls", then the "ls" command will be executed twice. I've literally been looking at my code for hours and have tried a bunch of stuff, but to no avail.

My code is displayed below:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#include<sys/wait.h>
#include<unistd.h>
#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <sys/types.h>
#include <sys/stat.h>

char* InputString();
char** GetArgs(char* com);
void Free(char** args);
char** GetCommands(char** line);
void PipedCommands(char* line);
int batch = 0; //Acts as bool for if there is a batch file given at command line
FILE* bFile; //This is just to make a quick tweek to the file if necssary to prevent any undefined behavior and keep track of where we are in the fil.
int b_fd;
int stdin_cpy;

int main(int argc, char** argv)
{
    pid_t pid;
    int status;
    int fd;
    int exitCom = 0; //acts as bool to check if an exit command was given.
    char* line;
    
    if(argc > 1) //check if batch file was given.
    {
        /*
        if(freopen(argv[1], "r", stdin) == NULL) //I added this in case the file isn't found
        {
            printf("\nCould not open \"%s\". Terminated\n\n", argv[1]);
            exit(1);
        }
        */
        //The following is to append a newline at the end of the file (if there isn't one).
        //For some reaosn, there seems to be some undefined behavior if the input file isn't
        //stricitly ended with a newline.
        bFile = fopen(argv[1], "r+"); //open for reading and updating
        if(bFile == NULL)
        {
            printf("\nCould not open \"%s\". Terminated\n\n", argv[1]);
            exit(1);
        }
        fseek(bFile, -1, SEEK_END); //go to last character of file
        if(fgetc(bFile) != '\n') //If last character is not a newline, append a newline to the file.
        {
            fprintf(bFile, "\n");
        }
        fclose(bFile); //close the file.
        
        bFile = fopen(argv[1], "r"); //open file to keep track of when it ends
        b_fd = open(argv[1], O_RDONLY); //open file again (with file descriptor this time) to duplicate it to stdin
        stdin_cpy = dup(fileno(stdin)); //keep track of stdin file.
        dup2(b_fd, 0); //duplicate to stdin so program takes input from bFile
        close(b_fd);
        batch = 1;
    }
    
    
    //int i=0; //this was used for debugging purposes
    while(1)
    {
        printf("\n");
        char** coms = GetCommands(&line);
        for(int i=0; coms[i] != NULL; ++i) //loop goes through each command returned from GetCommands(...)
        {
            //fork and wait.
            pid = fork();
            wait(&status);
            
            if(pid == 0)
            {
                int pipedCommand = 0;
                //printf("\ncoms[%d]: %s\n", i, coms[i]);
                for(int j=0; j<strlen(coms[i]); ++j)
                {
                    if(coms[i][j] == '|')
                    {
                        pipedCommand = 1;
                        break;
                    }
                }
                if(pipedCommand == 1)
                {
                    PipedCommands(coms[i]);
                    exit(1);
                }
                char** args = GetArgs(coms[i]);
                //printf("\nargs[0]: %s\n", args[0]);
                if(strcmp(args[0],"exit") == 0)
                {
                    exit(5); //if exit command was given, exit status will be 5 (I just used 5 becuse I felt like it).
                }
                //printf("\nNo exit\n");
                printf("\n");
                execvp(args[0],args);
                printf("%s: command not found\n", args[0]);
                exit(1); //Normal exit exits with 1 or 0.
            }
            //Parent continues after child exits
            else if(pid > 0)
            {
                //check exit status of child
                if(WEXITSTATUS(status) == 5)
                    exitCom = 1; //set bool exitCom to 1 (true), indicating that the exit command was given
            }
        }
        if(pid > 0)
        {
            free(line);
            free(coms);
            //Now that all commands in the line were executed, check exitCom and if it is 1 (exit command was given), the shell can now exit.
            if(exitCom == 1)
            {
                printf("\n");
                exit(0);
            }
        }
/*
        if(i >= 5)
        {
            printf("\nFORCED EXIT\n");  //this was used for debugging purposes
            exit(1);
        }
        ++i;
*/
    }

    return 0;
}

char* InputString()
{
    int len = 20;
    char* str = (char*)malloc(sizeof(char)*len);
    char* buff;
    unsigned int i=0;

    if(str != NULL)
    {
        int c = EOF;
        //printf("%c", fgetc(bFile));
        while( ((c = getchar()) != '\n') && (c != EOF) )
        {   
            /*
            //printf("%c", fgetc(bFile));
            //fgetc(bFile);
            if(feof(bFile))
            {
                printf("\n\nEnd of the line\n\n");
            }
            */
            str[i++] = (char)c;
            if(i == len)
            {
                len = len*2;
                str = (char*)realloc(str,sizeof(char)*len);
            }
        }
        str[i] = '\0';
        buff = (char*)malloc(i);
    }
    if(batch == 1)
    {
        if(fgets(buff, i, bFile) == NULL) //Once the end of file has been reached
        {
            dup2(stdin_cpy, 0); //revert input back to original stdin file so user can now enter commands interactively (this happens if exit command was not given)
            close(stdin_cpy); //close stdin_copy
            fclose(bFile); //close bFile as we have reached the end of it
            batch = 0;
        }
    }
    printf("\n");
    return str;
}

//User enters a line of commands (1 or more). Commands are separated with a ';' being the delimeter.
char** GetCommands(char** line)
{
    char** coms = (char**)malloc(sizeof(char*)); 
    char delim[] = ";";
    
    if(batch == 0)
        printf("prompt> ");
        fflush(stdout);
    
    *line = InputString();
    if(batch == 1)
        printf("%s\n", *line);
    
    strcat(*line, ";");
    
    int i=0;
   coms[i] = strtok(*line, delim);
   while(coms[i] != NULL)
   {
       ++i;
       coms = (char**)realloc(coms, sizeof(char*) * (i+1));
        coms[i] = strtok(NULL, delim);
        //printf("\ni: %d\n", i); 
   }
   
   return coms;
}
    

//A command obtained from GetCommands(...) is separated into various arguments with a space, ' ', being the delimiter.
char** GetArgs(char* com)
{
    
    char** args = (char**)malloc(sizeof(char*));
    char delim[] = " ";

    //printf("\nline: %s\n", line);
   int i=0;
   args[i] = strtok(com, delim);
   while(args[i] != NULL)
   {
       ++i;
       args = (char**)realloc(args, sizeof(char*) * (i+1));
        args[i] = strtok(NULL, delim);
   }
   
   return args;
}


void PipedCommands(char* line)
{
    char** coms = (char**)malloc(sizeof(char*));
    int numComs;
    char delim[] = "|";
    
    int i=0;
    coms[i] = strtok(line, delim);
   while(coms[i] != NULL)
   {
        ++i;
        coms = (char**)realloc(coms, sizeof(char*) * (i+1));
        coms[i] = strtok(NULL, delim);
   }
   numComs = i;
   
   int fd[2];
   pid_t pid;
   int status;
   int prev_p = 0;
    
  // printf("\nnumComs: %d\n", numComs);
    for(int i=0; i<numComs; ++i)
    {
        //printf("\ni: %d\n", i);
        pipe(fd);
        pid = fork();
        wait(&status);
        if(pid == 0)
        {
            //printf("\nChild\n");
            if(i < numComs-1)
            {
                //printf("\ni < numComs-1\n");
                //printf("%s", coms[i]);
                //printf("coms[%d]: %s", i, coms[i]);
                //printf("\nBefore dup2\n");
                char** args = GetArgs(coms[i]);
                //printf("\nexecvp in if\n");
                if(prev_p != 0)
                {
                    dup2(prev_p, 0);
                    close(prev_p);
                }
                dup2(fd[1], 1);
                close(fd[1]);
                execvp(args[0],args);
                printf("%s: command not found\n", args[0]);
                exit(3);
            }
            else
            {
                //printf("\nelse\n");
                //printf("coms[%d]: %s", i, coms[i]);
                //printf("\nBefore dup2 in else\n");
                if(prev_p != 0)
                {
                    dup2(prev_p, 0);
                    close(prev_p);
                }
                //close(fd[0]);
                close(fd[1]);
                char** args = GetArgs(coms[i]);
                printf("\n");
                execvp(args[0],args);
                printf("%s: command not found\n", args[0]);
                exit(3);
            }
        }
        close(prev_p);
        close(fd[1]);
        prev_p = fd[0];
        if(WEXITSTATUS(status) == 3)
        {
            close(fd[0]);
            close(prev_p);
            close(fd[1]);
            return;
        }
    }
    close(fd[0]);
    close(prev_p);
    close(fd[1]);
        
}


You can probably ignore the PipedCommands(...) function as I do not think the problem lies there.

Below is a simple batch file:

kldfg
whoami

Below is the output using the above batch file

kldfg
kldfg: command not found

whoami
jco0100

whoami
jco0100

The whoami command should only execute once, but it appears to execute twice. After that, the program reverts to interactive mode as it should and everything runs fine from there. Does anyone have any idea why this is happening. This only happens when an unknown command is entered. If all commands in the batch file are valid, nothing is outputted twice. It's only for batch files that have an unknown command that all commands after the unknown one are outputted twice.

Here's another example:

Batch file:

date
kldfg
whoami; ls | wc -l
date | wc -c

Output:

date
Tue Apr 13 19:43:19 CDT 2021

kldfg
kldfg: command not found

whoami; ls | wc -l
jco0100
34

date | wc -c
29

whoami; ls | wc -l
jco0100
34

date | wc -c
29
marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
  • 5
    Time to learn the debugger. If you're on linux you can start with using the gnu debugger `gdb` on the command line. This will let you step through the code to see what's happening. – Jevon Kendon Apr 14 '21 at 01:02
  • 3
    Please make this a [mcve], emphasis on minimal. – SuperStormer Apr 14 '21 at 01:03
  • 1
    _"looking at my code for hours and tried a bunch of stuff"_ is not a very useful description of your debugging process. In that time, you ought to have isolated the issue down to a specific part of your program that you can either fix, or you need help with understanding. Simply reading code and making guesses is not generally a practical approach, although there are advanced cases where that is the only option. – paddy Apr 14 '21 at 01:29
  • After a child process fails to execute `kdfg`, the child _must_ exit with a non-zero status. Otherwise, both the parent shell and the child continue interpreting the input. Superficially, it looks like you do that. Error messages should be report to `stderr` and not to `stdout`. – Jonathan Leffler Apr 14 '21 at 02:03
  • You have `pid = fork(); wait(&status); if (pid == 0)`. The `wait()` is dubious. It works because the child's call will fail whereas the parent's will hang until the child exits. But it is more sensible to just test the value of `pid` and to capture the PID of the exited process as well as the status when you do call `wait()`, in the parent code. You'll also be able to handle background commands better when the time comes. – Jonathan Leffler Apr 14 '21 at 02:08
  • Note that the indentation on `if(batch == 0) printf("prompt> "); fflush(stdout);` is misleading; the `fflush()` function is called regardless of the value in `batch`. That may be intentional, but the indentation is wrong. Or you need braces around the body of the `if`. – Jonathan Leffler Apr 14 '21 at 02:10
  • In testing on a Mac, after some minor cleanups, I got the batch file working and reading commands once. I'm not sure your problem is reproducible — but the behaviour on Linux might be different. – Jonathan Leffler Apr 14 '21 at 03:06
  • @JonathanLeffler Can try it on docker: `docker run -v c:\program-dir:c:/app -it gcc bash` – Jevon Kendon Apr 14 '21 at 03:09
  • Good error messages are important. `man perror` – William Pursell Apr 14 '21 at 04:20
  • The answers are helpful, but the question isn't currently written in a way that makes them easy to find (someone whose stdin is being taken over by a child process probably won't click on a link titled "issues with shell program in C" -- nothing about that title indicates what the problem actually is, and thus what the answers will describe how to fix). Now that you know what the "real" problem is, maybe edit the question to focus it in on that problem, so people with the same problem can find the answers and benefit from them? – Charles Duffy Jul 19 '21 at 20:47

3 Answers3

1

I got it working by disconnecting stdin on the child process before running the command:

...
freopen("/dev/null", "r", stdin); // disconnect
execcvp(args[0], args);
...

From this link: If I fork() and then do an execv(), who owns the console?

Jevon Kendon
  • 545
  • 4
  • 13
  • I am trying to understand why disconnecting stdin in the child fixes the issue. I think I have a very loose understanding of it, but I am not really sure. Could you please help me understand how this works? – joseph onuorah Apr 14 '21 at 06:48
  • I don't know either. We need a linux system guru. – Jevon Kendon Apr 14 '21 at 22:39
  • I would change your program. In batch mode just read commands from the file and then exit. No need to play with stdin. – Jevon Kendon Apr 14 '21 at 22:44
0

I had a crack at debugging it. To get you started on that road:

Compile your C program with debugging symbols:

$ gcc --debug your-program.c

Now debug your C program:

$ gdb a.out

This start a gdb interactive shell.

In gdb itself:

(gdb) list
(gdb) set follow-fork-mode parent
(gdb) breakpoint 69
  1. list your code
  2. tell the debugger to follow the parent when fork()
  3. set a breakpoint at line 69

Run the program:

(gdb) run batch.txt

It will pause at line 69. Execute next line:

(gdb) next

Print a variable:

(gdb) print *coms

Continue running:

(gdb) continue

I leave the rest for you to explore.

I'm still not sure what's wrong with it. Something strange happens in InputString() after your fork fails with an unknown command. InputString() begins returning duplicates from getchar().

I didn't even know you could do that with stdin. Maybe just read from the file in a normal fashion, rather than clobbering stdin, and see if the problem goes away.

Jevon Kendon
  • 545
  • 4
  • 13
  • My eyes are watering. We all start here, though, so please don't be discouraged or offended when I say find a book or website on how to structure a program. That's a skill. It takes time. – Jevon Kendon Apr 14 '21 at 02:39
-2

Don't write linux code much, I am trying to do more of that. I took the fork and wait commands out so I could build it in mingw64 (because they arent supported in windows builds) and can't seem to reproduce the issue.

So I think the issue is in the multi-threading setup you have going there.

The "pid" variable is shared between every fork. which means when the fork command is called "pid" is set to whatever the last fork in the loop returned.

It looks like you are using an if statement checking the "pid" variable to see if this thread can execute the command. But wouldn't the main thread keep running right through that?

I don't know what fork() returns but "pid" is uninitialized, don't know if that matters.

Maybe this helps?

  • 1
    `The "pid" variable is shared between every fork.` is not correct. The parent and child each have their own copy of `pid`. The `pid` value is how the process can 'know' if it's the parent or the child. – Jevon Kendon Apr 14 '21 at 04:21
  • You can run it on linux via docker. From windows: `docker run -v c:\program-dir:c:/app -it gcc bash` – Jevon Kendon Apr 14 '21 at 04:23
  • 1
    That would break my understanding of how this all works... pid is defined in the main() scope. which means it is not dynamically assigned a memory address but rather assigned that address at compile time. I don't see any code in this example which creates new copies for the children. Unless there's weird stack shenanigans with the fork() function. Edit: Huh Nevermind then. Apperently fork() creates an entire copy of the process new stack and all. That's super weird. – CatOfBlades Apr 14 '21 at 05:50
  • this may help: https://www.csl.mtu.edu/cs4411.ck/www/NOTES/process/fork/create.html – Jevon Kendon Apr 15 '21 at 04:39