Yet another shell in C

Question

I wrote this shell in C about a semester ago for a university assignment on operating systems. Even though I got a score of 10/10 in this assignment, I doubt it deserved it -

• The pipeline implementation could be written better.
• There were some cases (1%) where I got the pipeline to stuck while bash did not.
• Maybe some functions could be broken down into smaller/simpler ones.
• The debug messages (although they were optional) could be written in a log file.

We were not allowed to split the source code into separate files

Here is the source code for it.

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

struct subcommand_t {
    char **argument; // Array of arguments
    size_t size;     // Number of arguments
};

struct command_t {
    struct subcommand_t *subcommand; // Array of subcommands.
    size_t size;                     // Number of subcommands.
};

struct line_t {
    struct command_t *command; // Array of commands.
    size_t size;               // Number of commands.
};

void prompt(char *prompt) {
    #ifdef DEBUG
    fprintf(stderr, "[PROMPT] Displaying prompt\n");
    #endif
    fprintf(stdout, "%s ", prompt);
}

char *reader(size_t size){
    char *buffer;
    #ifdef DEBUG
    fprintf(stderr, "[READER] Allocating memory for buffer [size: %zu]\n", size);
    #endif
    buffer = malloc(sizeof(char) * size);
    if (buffer == NULL) exit(EXIT_FAILURE);
    #ifdef DEBUG
    fprintf(stderr, "[READER] Reading to buffer\n");
    #endif
    int character;
    size_t length = 0;
    while (EOF != (character = fgetc(stdin)) && character != '\n') {
        buffer[length++] = (char) character;
        if (length == size) {
            #ifdef DEBUG
            fprintf(stderr, "[READER] Reallocating memory for buffer [size: %zu]\n", size);
            #endif
            buffer = realloc(buffer, sizeof(char) * (size += 32));
            if (buffer == NULL) exit(EXIT_FAILURE);
        }
    }
    #ifdef DEBUG
    fprintf(stderr, "[READER] Setting the NULL terminator for buffer\n");
    #endif
    buffer[length++] = '\0';
    return realloc(buffer, sizeof(char) * length);
}

void executor(struct line_t line) {
    for (size_t i = 0; i < line.size; ++i) {
        #ifdef DEBUG
        fprintf(stderr, "[EXECUTOR] Executing line.command[%zu]\n", i);
        #endif
        int previous;
        for (size_t j = 0; j < line.command[i].size; ++j) {
            #ifdef DEBUG
            fprintf(stderr, "[EXECUTOR] Executing line.command[%zu].subcommand[%zu]\n", i, j);
            #endif
            int p[2];
            pipe(p);
            if (line.command[i].subcommand[j].size != 0) {
                if (strcmp(line.command[i].subcommand[j].argument[0], "exit") == 0) {
                    #ifdef DEBUG
                    fprintf(stderr, "[EXECUTOR] Exiting shell\n");
                    #endif
                    exit(EXIT_SUCCESS);
                } else if (strcmp(line.command[i].subcommand[j].argument[0], "cd") == 0) {
                    #ifdef DEBUG
                    fprintf(stderr, "[EXECUTOR] Changing directory to %s\n", line.command[i].subcommand[j].argument[1]);
                    #endif
                    chdir(line.command[i].subcommand[j].argument[1]);
                } else {
                    switch(fork()) {
                        case -1:
                            #ifdef DEBUG
                            fprintf(stderr, "[EXECUTOR] Failed to fork a child process\n");
                            #endif
                            exit(EXIT_FAILURE);
                        case  0:
                            #ifdef DEBUG
                            fprintf(stderr, "[EXECUTOR] Running on a child process [pid: %d]\n", getpid());
                            #endif
                            if (j == 0) {
                                dup2(0, STDIN_FILENO);
                            } else {
                                dup2(previous, STDIN_FILENO);
                            }
                            close(p[0]);
                            if (j+1 < line.command[i].size) {
                                dup2(p[1],STDOUT_FILENO);
                            }
                            close(p[0]);
                            execvp(line.command[i].subcommand[j].argument[0], line.command[i].subcommand[j].argument);
                            exit(EXIT_FAILURE);
                        default:
                            #ifdef DEBUG
                            fprintf(stderr, "[EXECUTOR] Running on the parent process [%d]\n", getpid());
                            #endif
                            previous=p[0];
                            close(p[1]);
                    }
                }
            }
        }
        #ifdef DEBUG
        fprintf(stderr, "[EXECUTOR] Waiting for any terminated child processes\n");
        #endif
        while(wait(NULL) > 0) {
            #ifdef DEBUG
            fprintf(stderr, "[EXECUTOR] Found a terminated child process\n");
            #endif
        }
    }
}

struct line_t parser(char *buffer, char *del1, char *del2, char *del3) {
    size_t i, j, k;
    char *str1, *str2, *str3;
    char *token1, *token2, *token3;
    char *saveptr1, *saveptr2, *saveptr3;

    struct line_t line;
    line.size=0;
    line.command=NULL;
    for (i = 0, str1 = buffer ;; i++, str1 = NULL) {
        token1 = strtok_r(str1, del1, &saveptr1);
        if (token1 == NULL) break;
        line.size++;
        if (i == 0) {
            #ifdef DEBUG
            fprintf(stderr, "[PARSER] Allocating memory for line.command [size: %zu]\n", line.size);
            #endif
            line.command = malloc(sizeof(struct command_t));
        } else {
            #ifdef DEBUG
            fprintf(stderr, "[PARSER] Reallocating memory for line.command [size: %zu]\n", line.size);
            #endif
            line.command = realloc(line.command, line.size * sizeof(struct command_t));
        }
        line.command[i].size=0;
        line.command[i].subcommand=NULL;
        for (j = 0, str2 = token1 ;; j++, str2 = NULL) {
            token2 = strtok_r(str2, del2, &saveptr2);
            if (token2 == NULL) break;
            line.command[i].size++;
            if (j == 0) {
                #ifdef DEBUG
                fprintf(stderr, "[PARSER] Allocating memory for line.command[%zu].subcommand [size: %zu]\n", i, line.command[i].size);
                #endif
                line.command[i].subcommand = malloc(sizeof(struct subcommand_t));
            } else {
                #ifdef DEBUG
                fprintf(stderr, "[PARSER] Reallocating memory for line.command[%zu].subcommand [size: %zu]\n", i, line.command[i].size);
                #endif
                line.command[i].subcommand = realloc(line.command[i].subcommand, line.command[i].size * sizeof(struct subcommand_t));
            }
            line.command[i].subcommand[j].size=0;
            line.command[i].subcommand[j].argument=NULL;
            for (k = 0, str3 = token2 ;; k++, str3 = NULL) {
                token3 = strtok_r(str3, del3, &saveptr3);
                if (token3 == NULL) break;
                line.command[i].subcommand[j].size++;
                if (k == 0) {
                    #ifdef DEBUG
                    fprintf(stderr, "[PARSER] Allocating memory for line.command[%zu].subcommand[%zu].argument [size: %zu]\n", i, j, line.command[i].subcommand[j].size);
                    #endif
                    line.command[i].subcommand[j].argument = malloc(sizeof(char *));
                } else {
                    #ifdef DEBUG
                    fprintf(stderr, "[PARSER] Reallocating memory for line.command[%zu].subcommand[%zu].argument [size: %zu]\n", i, j, line.command[i].subcommand[j].size);
                    #endif
                    line.command[i].subcommand[j].argument = realloc(line.command[i].subcommand[j].argument, (line.command[i].subcommand[j].size + 1) * sizeof(char *));
                }
                line.command[i].subcommand[j].argument[k] = malloc((strlen(token3)+1) * sizeof(char));
                memset(line.command[i].subcommand[j].argument[k], 0, strlen(token3)+1);
                strcpy(line.command[i].subcommand[j].argument[k], token3);
            }
            if (line.command[i].subcommand[j].size != 0) {
                #ifdef DEBUG
                fprintf(stderr, "[PARSER] Setting the NULL terminator for line.command[%zu].subcommand[%zu]\n", i, j);
                #endif
                line.command[i].subcommand[j].argument[line.command[i].subcommand[j].size] = NULL;
            }
        }
    }
    return line;
}

int main() {
    while (1) {
        prompt("$");
        char *buffer = reader(1024);
        struct line_t line = parser(buffer, ";", "|", " \t");
        executor(line);
    }
}

Answer 1

You wrote a nice and simple shell. It works for very simple commands but fails on more complex ones (for details, see below). The code can be cleaned up in several places.

---

To make your code nicely readable by others, let an automatic formatter take care of the indentation and spacing. If you have GNU Indent available, use the following command line:

indent --k-and-r-style --no-tabs --line-length 200 --case-indentation 4 --braces-on-func-def-line shell.c

---

Instead of writing this every time:

#ifdef DEBUG
fprintf(stderr, "[READER] Allocating memory for buffer [size: %zu]\n", size);
#endif

you should define a macro DEBUG_PRINTF:

#ifdef DEBUG
#define DEBUG_PRINTF(...) fprintf(stderr, __VA_ARGS__)
#else
#define DEBUG_PRINTF(...) (void)0
#endif

Then you can just write:

DEBUG_PRINTF("[READER] Allocating memory for buffer [size: %zu]\n", size);

---

if (buffer == NULL) exit(EXIT_FAILURE);

Before exiting with an error code, you should print an error message. By convention, an empty output means success.

---

Enable all compiler warnings and fix them properly. For GCC, these are -Wall -Wextra -Werror -O2.

• Instead of void prompt, write static void prompt. This makes the function local to the current file and avoids conflicts just in case another file defines another function of the same name. (Also for the other functions.)

• Instead of int main(), write int main(void) to fix the "missing prototype" warning.

• Declare all read-only strings as const char * instead of char *. This affects the parameters to prompt and to parser.

---

Rename your functions. Function names are usually verbs. Your current names are executor, parser, etc. These are readable and understandable but still should be execute and parse.

For reader, this is more difficult since there is a system-provided function called read. Therefore it should be renamed to read_line.

---

Declare variables directly where you need them. Instead of:

char *buffer;
buffer = malloc(sizeof(char) * size);

just write:

char *buffer = malloc(sizeof(char) * size);

Since sizeof(char) is defined to always be 1, leave it out:

char *buffer = malloc(size);

---

In the executor function, instead of repeating line.command[i].subcommand[j] everywhere, define a new variable:

for (size_t j = 0; j < line.command[i].size; ++j) {
    subcommand_t subcommand = line.command[i].subcommand[j];

    if (subcommand.size != 0) {
        // ...

---

The parser function currently parses the following command unexpectedly:

echo ";"

It outputs " but doesn't give any hint on why the semicolon and the second quotation mark were not echoed.

---

memset(line.command[i].subcommand[j].argument[k], 0, strlen(token3)+1);
strcpy(line.command[i].subcommand[j].argument[k], token3);

The memset is redundant and should be removed.

---

Any memory that you allocate with malloc or realloc must be freed after use with free.

---

When I press Ctrl+D in your shell, I get stuck in an endless loop. All other shells exit at that point (or instruct me that I must type exit instead of just Ctrl+D, which is really annoying).

To handle this situation, add error handling in the reader function and return from main if its return value is NULL.

Answer 2

Use of _t

You may want to reconsider use of _t endings in your type names. In short, most of the standard type names use this, and the general practice is to not do so with user-defined types.

To quote POSIX:

To allow implementors to provide their own types, all conforming applications are required to avoid symbols ending in "_t", which permits the implementor to provide additional types.

As a basic example:

typedef struct Subcommand {
    char **argument; // Array of arguments
    size_t size;     // Number of arguments
} Subcommand;

There's also more information in this Stack Overflow question that may be helpful.