Fork and execute

[jjordan33]

Hi, so I'm slowly trying to wrap my head around creating my own shell simulator. As far as I can understand it, the general steps are to:

loop driver to continually ask for input
taking the input and creating child processes with the fork command
then executing it.

I know there's a lot more to it, but I'm just starting with the bare bones first.

I have C code that will read a line of input and put it into a *char array, and I'm trying to fork it, then execute it.

I get a segmentation fault. I don't know if it's because I'm using the commands incorrectly or it's because I need a lot more code for a proper shell.

(As of now, I'm just forking immediately. I've yet to parse anything, do any error checks, whatever piping is, and checking for built-in prompts.)

Am I using fork and execute wrong or do I just need more code for those two specifically to work?

#include <unistd.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>

void driveLoop();
char *userInput(void);

int main(int argc, char **argv){
    driveLoop();
    return 0;
}

void driveLoop(void){

    char *command;
    char **processPtrs;
    int x;
    int loop = 1;

    while (loop == 1){

        printf("> ");
        command = userInput();

        for (int i = 0; i < 10; i++){

            processPtrs[i] = strsep(&command, " ");
            if (processPtrs[i] == NULL){
                break;
            }

        }

    }

    pid_t cFork = fork();
    execvp(processPtrs[0], processPtrs);

}

char *userInput(void){
    char *input = NULL;
    size_t size = 0;
    getline(&input, &size, stdin);
    
    return input;

}

typing in "date" or "l", etc. produces the segmentation fault.

Thank you.

[dutch]

The code you've posted doesn't even compile. Please post real code. And make sure you fix all warnings (compiling with -Wall -W -pedantic).

You're definitely not using fork() correctly, but until you post code that compiles ....

BTW, "l" is not a command but just an alias (presumably). You need to say "ls" instead. Aliases are handled by the actual shell, which you are of course not using here.

[jjordan33]

I must've made a typo copying it from the linux terminal to my windows chrome. One sec.

Thanks for the tip about aliases. I didn't know that.
*edit*

Okay, fixed. There are 2 warnings, but it compiles now. I left off the f in printf.

*second edit*

Got rid of the warnings as well.

[dutch]

The warning was far more important, wasn't it! :)
Anyway, this works for me:

#define _BSD_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
#include <unistd.h>
#include <sys/wait.h>

char *userInput() {
    char *input = NULL;
    size_t size = 0;
    getline(&input, &size, stdin);
    return input;
}

void driveLoop() {
    char *toks[100]; // max of 99 tokens (last must be NULL)

    for (;;){

        printf("> ");
        char *command = userInput();
        if (!*command)
            break; // exit if empty string (Ctrl-D)

        int i = 0;
        char *tok = strtok(command, " \t\n");
        while (tok && i < 99) {
            toks[i++] = tok;
            tok = strtok(NULL, " \t\n");
        }
        toks[i] = NULL;

        pid_t cFork = fork();
        if (cFork == (pid_t)-1)
            perror("fork");
        else if (cFork == 0) { // child
            execvp(toks[0], toks);
            perror("exec"); // probably command not found
        }
        else // parent
            wait(NULL);
    }
}

int main(){
    driveLoop();
    return 0;
}

[jjordan33]

Definitely was lol. I'm still not used to using emacs and compiling in terminal. I forget warnings aren't a thing that just happens.

So, I looked up strtok as opposed to my strsep.

Does this mean that fork has to be called on an array of chars pointing to tokens?

[dutch]

strtok and strsep and almost identical. The reason I switched to strtok is because strsep creates empty tokens for multiple delimiters in a row, which you probably don't want.

fork() is of course called with no arguments.
But, yes, execvp is called with an array of tokens. That's what the 'v' stands for (vector/array). The 'p' means that it uses the path so that you don't need to enter the full path to the executables.

Note that fork literally "forks" the program into two programs, the child and the parent. The only difference is that it returns 0 to the child and the (non-zero) pid to the parent. The child performs the exec, which completely replaces the process with the executable, and the parent calls wait to wait until the child finishes (we wouldn't do that if we wanted to run the child "in the background"). The parent should probably collect the child's return status by not just passing NULL, though.

[jjordan33]

Okay, I'll look into the wait() and how to use it to collect return status.

Thank you.

[kbw]

char *userInput() {
    char *input = NULL;
    size_t size = 0;
    getline(&input, &size, stdin);
    return input;
}

Are you guys ok with this? It should be obvious that it's wrong.

[dutch]

kbw wrote:

It should be obvious that it's wrong.

Not as obvious as you think. It's not C++. It's C, if that's what's confusing you.
For details, see: https://linux.die.net/man/3/getline

[jjordan33]

Would that function being wrong have anything to do with the "shell" not recognizing the built-in quit command?

[dutch]

That function isn't wrong. Built-in commands are commands that you have to implement yourself (they aren't called with fork/exec).

If you are trying to compare the return value of getline using strcmp then remember that getline leaves the newline at the end of the string, so it would probably contain "quit\n" not just "quit".

It would probably be best to do the strcmp after the tokenization loop, comparing toks[0] with "quit". The tokenizing loop will remove the newline and other whitespace.

[jjordan33]

I was mistaken. The built-in command is exit, not quit.

I wasn't aware you had to implement them specifically. Okay.
That's something I can work with.

Thank you.

**edit**

Okay. So, am I reading it wrong, or do I simply just say, "hey, the char array just stored the word 'exit'. I need to return 0 and end the program"?

pseudo:

if (array[0] == "exit")
return 0;

[dutch]

After the tokenizing loop, but before the fork, use strcmp to test the string value:

    if (strcmp(toks[0], "exit") == 0)
        break;

[jjordan33]

Thanks a lot.

[kbw]

Not as obvious as you think. It's not C++. It's C, if that's what's confusing you.

You're right.

[jjordan33]

Hi, is this code passable? I've written workable code before that was, and I quote, unacceptable.

if (strcmp(toks[0], "cd") == 0){

   cd = getcwd(cdBuf, sizeof(cdBuf); //cd and cdBuff[1000] dynamic char arrays
   cdDir = strcat(cd, "/"); // char *cdDir
   cdTemp = strcat(cdDir, toks[1]); //char *cdTemp
   chdir(cdTemp);
   continue;

}

[kbw]

Assuming the buffer is large enough, and the directory exists, then yes.

But your expecting that code to be in a loop. I have no idea where you intent to put it.

To fix those assumptions you've made:
1. Use strncat/strncpy instead of strcat/strcpy to avoid buffer overruns.
2. Check the return code from chdir() to check for errors.

[jjordan33]

Got it. Thanks a lot y'all.

[jjordan33]

I'm having trouble incorporating this code into a buffer that reads line by line data from a file.
i.e.
date
cd
etc.

I verified the file is read in with a print, so I'm pretty confident the issues lies with the new line character. It's being treated as all one command line, and I don't know enough C to be able to fix it.

#include<unistd.h>
#include<stdio.h>
#include<stdlib.h>
#include<string.h>
#include<errno.h>


#define bSize 1000


void driveLoop();
char *userInput(void);
void removeExit(char *original, char *subString);
void batchMode(char *c);


int main(int argc, char **argv){




  char *fTemp;
  if (argc == 1)
    driveLoop(); 


  else if (argc == 2)
    batchMode(&argv[1][0]);
            
  
  return 0;


}


void driveLoop(void){
  char *comTokens[100];
  char *tempTokens;
  char *command;
  char *cd;
  char *cdDir;
  char* cdTemp;
  char cdBuf[bSize];
  char checkExit[] = "exit";
  
  for (;;){


    printf("> ");
    command = userInput(); 


    if (!*command) 
      break;


    char *exitPtr = strstr(command, "exit"); 
    removeExit(command, "exit"); 
    puts(command); 
    
    int i = 0;
    tempTokens = strtok(command, " \t\n"); 


    while (tempTokens && i < 99){ 
      comTokens[i++] = tempTokens;
      tempTokens = strtok(NULL, "\t\n");
    }


    if (strcmp(comTokens[0], "exit") == 0) // exit if input is "exit" only
      exit(0); 




    if(strcmp(comTokens[0], "cd") == 0){ // built in change directory command
      cd = getcwd(cdBuf, sizeof(cdBuf));
      cdDir = strcat(cd, "/");
      cdTemp = strcat(cdDir, comTokens[1]); // cplusplus.com reference
      chdir(cdTemp);
      continue;
    }
    
    comTokens[i] = NULL;


    pid_t cFork = fork(); // creates duplicate child process of parent


    if (cFork == (pid_t) - 1){ // error check
      perror("fork");
    }


    else if (cFork == 0) { 
      execvp(comTokens[0], comTokens);
      perror("exec");      
    }
    
    else { // children are returned. parent executes
      int status;
      waitpid(cFork, &status, 0);
      if (exitPtr != NULL){ // if substring exit was found, exit the program
    exit(0);
      }
    }


  }
  
}


char *userInput(void){  // referenced Linux man page - getline(3) (linux.die.net)
  char *input = NULL;
  size_t size = 0;
  getline(&input, &size, stdin); // updates the size as it goes along
  return input;
}


void removeExit(char *original, char *subString){ // removes exit from string
  char *ex;
  int len = strlen(subString); 
  while ((ex = strstr(original, subString))){ // Referenced from a Stack Overflow page.
    *ex = '\0';
    strcat(original, ex+len);
  }
}


void batchMode(char *c){


  char *tok[100];
  char *batchTokens;
  char *batchBuffer = NULL;
  size_t batchSize = 0;


  FILE *fp = fopen(c, "r");


  unsigned int line = 1;
  char buffer[bSize];


  while(fgets(buffer, sizeof(buffer), fp)){
      
      int i = 0;


      char *toks = strtok(buffer, "\t\n");


      while (toks && i < 99){
           tok[i++] = toks;
           toks = strtok(NULL, " \t\n");
      }


      tok[i] = NULL;
          pid_t bFork = fork();


      if (bFork == (pid_t) - 1)
          perror("fork");


      else if (bFork == 0){
         execvp(tok[i], tok);
         perror("exec");
      }


      else {
        int status;
        waitpid(bFork, &status, 0);
      }
    
  }
  
}

*edit*
Disregard. The issue was here.

while (fgets (buffer, sizeof(buffer), fp)) {

        int i = 0;
        char *tok[100];
        char *toks = strtok(buffer, " \t\n");

        while (toks && i < 99){
            tok[i] = malloc (strlen(toks) + 1);
            strcpy(tok[i++], toks);
            toks = strtok(NULL, " \t\n");
        }

        tok[i] = NULL;
            pid_t bFork = fork();

        if (bFork == (pid_t) - 1)
            perror("fork");
        else if (bFork == 0){
            execvp (tok[0], tok);
            perror("exec");
        }
        else {
            int status;
            waitpid(bFork, &status, 0);
        }

Thank you.

[elb]

There's one issue that's not been mentioned yet, and that as to do with the use of getline(). getline() uses malloc() to get space for the output buffer, so by not calling free(), you're constantly loosing memory.
The quick way to solve that is to do it in your for() statement e.g.

for( ;; free(command)) {

However, now you are constantly calling malloc()/free(), and we can do better. The first time you call getline(), using a given buffer, you need to make sure that len parameter is 0. Subsequent calls. using the same buffer, use the same len parameter, and getline() will only allocate more memory when it needs to. getline also returns the number of chars read (not including the final '\0'). Knowing that we can do the following:
1) pass the buffer and its allocated length into userInput(), so getline() can reuse the buffer.
2) use the returned length from getline() to trim off the trailing '\n' in the input
3) if we return the value of getline(), we can test for -1, which is returned on error, including EOF. This means an input of CTL-D can be used to end the shell.

Since getline() calls malloc() only when it needs to, we have gained some efficiency, and now we only have to call free() once!

So what we have is the following changes:

ssize_t userInput(char **buffer, size_t *buflen)
{
        ssize_t  rlen =  getline(buffer, bufflen, stdin);
        if(rlen > 0) /* trim off trailing newline */
                buff[rlen--] = '\0';
        return rlen;
}

void driverLoop(void)
{
        /* ... */
       char *command = NULL;
       size_t lenCommand = 0;;
       /* ... */
       
       for(;;) {
               ssize_t bytesRead = 0;
               fputs("> ", stdout);  /* avoids all the printf overhead.  */
          
               bytesRead = userInput(&command, &lenCommand);
               if( bytesRead == -1 ) {
                        break;    /* EOF? maybe we should do some error checking? */
               } else if (bytesRead == 0)  { /* empty input, can continue */
                        continue;
               }
              
               /* ... */
              
        }
        free(command);
}

You might also consider using vfork() rather than fork(), since you don't do anything but call execvp()[/t]t immediately after. Using [tt]vfork() means that you don't copy the program memory pages, which could be a win if this gets large. The usual way of making sure that nothing but an exec gets called after [tt]vfork[/t] is something like this:

pid_t child;
if( (child = vfork()) == 0) ) {
         execvp( /* args */ );
         perror("exec");
         exit(1);   /* otherwise we get an Abort, core dumped! even for fork! */
}
else if ( child == -1 ) { /* note we don't need to cast to pid_t here */
        perror("fork");
}
else {
        /* parent waits for child */
        wait( /* args */ );
}

oops the definition of userInput was ssize_t userInput[char **buffer, size_t bufflen). That was wrong. bufflen should have been a size_t*. Fixed. my bad.