C implementation of a minimal shell

Question

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));
 }
}

Answer 1

• 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]);
   }
  

• Mark: How can I rezolve the overflow?