3
\$\begingroup\$

I wrote a program that implements a minimal shell and reads from standard input commands and executes them.

Is my implementation ok? I admit that I helped myself a lot with google. Could I have written a better implementation?

Also, I tried to check the program with the dash command, because they are similar. I tried to close the terminal with the key combination Ctrl + D, but it displays a different error and exit status. Why?

code:

#include <unistd.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <sys/wait.h>
#define PRMTSIZ 255
#define MAXARGS 63
#define EXITCMD "exit"
int main()
{
    for (;;)
    {
        char input[PRMTSIZ + 1] = {0x0};
        char * ptr = input;
        char * args[MAXARGS + 1] = {NULL};
        int wstatus;
        printf("%s", getuid() == 0 ? "# " : "$ ");
        // get data from user
if(fgets(input, PRMTSIZ, stdin) == NULL)
        {
            close(1);
            exit(0);
        };
        if (*ptr == '\n')continue;
        const char delims[3] = " \n";
        /* get the first token */
        char * token = strtok(input, delims);
        /* walk through other tokens */
        for (int i = 0; token != NULL; i++)
        {
            args[i] = token;
            token = strtok(NULL, delims);
        }
        if (strcmp(EXITCMD, args[0]) == 0 ) return 0;
        // execute the command in child
        if (fork()==0)exit(execvp(args[0],args));
        // parent waits for child to complete before continuing
        wait(&wstatus);
        if (!WIFEXITED(wstatus)) printf("<%d>", WIFEXITED(wstatus));
    }
}
\$\endgroup\$

1 Answer 1

5
\$\begingroup\$
  • if (*ptr == '\n') is very opportunistic, and leads to a serious problem. An input like \n results in a segfault. You should honestly tokenize any input, and only then test what args[0] is.

  • It is easy to overflow an args array (try a a a ... for more than 64 times). A tokenizing loop should hard stop at 63rd token.

  • ptr is not really used anywhere.

  • wstatus is better be declared where it is used:

      int wstatus;
      wait(&wstatus);
      if (!WIFEXITED(wstatus)) printf("<%d>", WIFEXITED(wstatus));
    
  • You shall not ignore fork errors. As a minimum,

      switch (fork()) {
          default:
              normal_parent_code(); // Like, wait()
              break;
          case 0:
              child_code();
              break; // or exit()
          case -1:
              handle_fork_error(); // At least. print strerror
              break;
    
  • I don't see any value in printing WIFEXITED(wstatus). Much better diagnostic would be

      if (fork() == 0) {
          execvp(args[0],args);
          perror(args[0]);
      }
    
\$\endgroup\$
1
  • \$\begingroup\$ How can I rezolve the overflow? \$\endgroup\$
    – Mark
    Commented Mar 29, 2022 at 9:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.