Basic shell in C

Question

I'm new to C and created this shell to test my knowledge. It takes input from stdin, checks whether the full path has been given, if not, appends input to /bin/ and then executes. How can I improve; what are your suggestions?

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

// Takes input from stdin 
char *read_line(char *buf, size_t sz){

    printf("Enter command: ");
    if ( fgets(buf, sz, stdin) == NULL ) {
        return NULL;
    }
    buf[strlen(buf) - 1] = '\0';
}

// Splits input from stdin
void split(char *buf, char **split[], size_t max){

    char* token = strtok(buf, " \n");
    int index = 0;

    while(token != NULL){
        split[index] = malloc(strlen(token) + 1);
        strcpy(split[index], token);
        index++;
        token = strtok(NULL, " \n");
    }
}


int main(){

    char userInput[50];
    size_t szUI = sizeof(userInput); 

    char *inputSplit[50];
    size_t szIS = sizeof(inputSplit); 
    memset(inputSplit, 0, szIS);

    if ( read_line(userInput, szUI) == NULL ) {
        printf("Error: CTRL-D ENTERED");
        return 0;
    }

    split(userInput, inputSplit, szIS);

    while(1){

        if ( fork() == 0 ) {

            // Checks if / is present 
            if ( strpbrk(inputSplit[0], "/") != 0 ) { 
                if ( execv(inputSplit[0], inputSplit) == -1 ) {
                    perror("Error");
                    return 0;
                }
            } 
            // Otherwise / is not present
            char tmp[20] = "/bin/";
            strcat(tmp, inputSplit[0]);

            if ( execv(tmp, inputSplit) == -1 ) {
                perror("Error");
                return 0;
            }

            memset(tmp, 0, sizeof(tmp));
            exit(0);
        } 

        wait(NULL);

        memset(userInput, 0, szUI);
        memset(inputSplit, 0, szIS);

        char cont[20];
        printf("Continue?(y/n) ");

        if ( fgets(cont, sizeof(cont), stdin) ==  NULL ) {
            return 0; 
        }

        cont[strlen(cont)-1] = '\0';

        if ( strcmp(cont, "y") != 0 ) {
            return 0;
        }

        memset(cont, 0, sizeof(cont));

        if ( read_line(userInput, szUI) == NULL ) {
            printf("Error: CTRL-D ENTERED \n");
            return 0;
        }

        split(userInput, inputSplit, szIS);
    }   
}

Answer 1

Avoid a hacker exploit:

What happens when the first character read is a null character?

if ( fgets(buf, sz, stdin) == NULL ) {
    return NULL;
}
buf[strlen(buf) - 1] = '\0';  // same as buf[SIZE_MAX] = '\0';

Instead, use

buf[strcspn(buf, "\n")] = '\0';

---

Expand white-space list to the standard white spaces or use isspace()

// char* token = strtok(buf, " \n");
char* token = strtok(buf, " \n\t\r\v\f");`

---

What is the point of max in void split(char *buf, char **split[], size_t max){? I'd expect some check in code and a matching type for i

// int index = 0;
// while(token != NULL){
size_t index = 0;
while(token != NULL && i < max) {

---

Potential over-run. Why the magic 20?

// char tmp[20] = "/bin/";
char tmp[strlen(inputSplit[0]) + 5 + 1] = "/bin/";  // still not elegant
strcat(tmp, inputSplit[0]);

---

Flush after output to insure stdout is emptied before reading user input

printf("Continue?(y/n) ");
fflush(stdout);  // add
if ( fgets(cont, sizeof(cont), stdin) ==  NULL ) {

---

Replace with read_line(char *buf, size_t sz) with something more generic

char *read_line(const char *prompt, char *buf, size_t sz) {
  if (prompt) {
    fputs(prompt, stdout);   
    flush(stdout);
  }
  if (fgets(buf, sz, stdin) == NULL) {
    return NULL;
  }
  buf[strcspn(buf, "\n\r")] = '\0';
  return buf;
}

And use it in various places. note () not need with sizeof object.

  // printf("Continue?(y/n) ");
  // if ( fgets(cont, sizeof(cont), stdin) ==  NULL ) { return 0;  }
  // cont[strlen(cont)-1] = '\0';
  if (read_line("Continue?(y/n) ", cont, sizeof cont) == NULL) {  
    return 0;
  }

Your use of a helper function to handle user text input is a very good idea.

---

String duplication begs for a strdup() function that also checks for allocation success.

split[index] = strdup(token);
if (split[index] == NULL) return EXIT_FAILURE;

---

On failure from main(), do not return 0, which implies success. Use EXIT_FAILURE. Send error messages to stderr

#include <stdlib.h>
...
   if ( read_line(userInput, szUI) == NULL ) {
        //printf("Error: CTRL-D ENTERED \n");
        //return 0;
        fprintf(stderr, "Error: CTRL-D ENTERED \n");
        return EXIT_FAILURE;

---

Consider allowing a case insensitive response

// if ( strcmp(cont, "y") != 0 ) {
if ( strcmp(cont, "y") != 0 strcmp(cont, "Y") != 0 ) {

Or even "yes", etc.