8
\$\begingroup\$

I wrote a shell to test my understanding of processes etc. It is not supposed to comply with POSIX or anything, just to allow people to run simple commands with simple arguments.

#include <stdio.h>  /* printf, fprintf */
#include <stdlib.h> /* exit */
#include <string.h> /* strtok */
#include <unistd.h> /* fork, execvp */
#include <sys/wait.h>   /* waitpid */
#include <sys/types.h>  /* waitpid */
/*
 * This program is a simple, first-level shell for testing and development purposes.
 * It is called rsh: the Recovery SHell because it can be used for recovery as it has
 * no dependencies apart from the C standard library and is very small and simple.
 * This shell does not follow POSIX or anything else: it is purely for executing
 * commands with simple arguments (ie no quote escape, backslash escape, etc)
 */

/*
 * Function Return Values:
 * All functions in this program return 0 on success and -1 on failure, unless they
 * return useful information, in which the return values will be documented in a
 * comment at the top of the function. With the exception of builtins. Builtin
 * commands return 0 for success and 1 for not successful, like the real commands.
 */

/* A list of builtin functions here */
char *builtins[] = {
    "exit",
    "cd",
    "help"
};

/*
 * Return the number of builtin commands so we can avoid hard-coding random magic
 * numbers and other annoyances everywhere.
 */
int rsh_num_builtins(void)
{
    return sizeof(builtins) / sizeof(char *);
}

/*
 * This builtin function exits the shell.
 */
int rsh_exit(char **args)
{
    exit(EXIT_SUCCESS);
}

/*
 * This builtin function changes the directory of the shell process, like the cd
 * in normal shells.
 */
int rsh_cd(char **args)
{
    if (args[1] == NULL) {
        fprintf(stderr, "rsh: expected argument to cd\n");
        return 0;
    } else if (chdir(args[1]) == -1) {
        fprintf(stderr, "rsh: can't cd to %s\n", args[1]);
        return 1;
    }

    return 0;
}

/*
 * Print out a help message about rsh
 */
int rsh_help(char **args)
{
    puts("rsh: recovery shell");
    puts("");
    puts("rsh is a very small and simple shell that is used for system recovery purposes.");
    puts("It is not compliant with anything and does not allow escaping, quoting, sourcing");
    puts("backslashing or anything else. It is scriptable, but only in such a way that lets");
    puts("you run commands with simple options and arguments: nothing else!");
    puts("");
    puts("List of builtin commands:");
    puts("exit cd help");
    puts("All those builtins do exactly what you expect them to do, so no documentation is");
    puts("needed, hopefully.");

    return 0;
}

int (*builtin_func[]) (char **) = {
    &rsh_exit,
    &rsh_cd,
    &rsh_help
};

int rsh_external_execute(char **args)
{
    pid_t pid;
    pid_t wpid;
    int status;

    /* fork off a new process */
    pid = fork();

    if (pid == 0) {
        /* in the child process. Note that this does not exit the parent
         * in an error, with the exit() call. */
        if (execvp(args[0], args) == -1) {
            fprintf(stderr, "rsh: cannot exec\n");
            exit(EXIT_FAILURE);
        }
    } else if (pid < 0) {
        /* error forking */
        fprintf(stderr, "rsh: cannot fork\n");
        return -1;
    } else {
        /* parent process */
        do {
            wpid = waitpid(pid, &status, WUNTRACED);
        } while (!WIFEXITED(status) && !WIFSIGNALED(status));
    }

    return 0;
}

/*
 * This function handles execution of code and builtins. It takes the argument argv
 * which is the argv of the program about to be executed. The fork-exec process
 * takes place in the function rsh_external_execute().
 */
int rsh_execute(char **args)
{
    int i;

    if (args[0] == NULL) {
        /* An empty command was entered */
        return 1;
    }

    for (i = 0; i < rsh_num_builtins(); i++) {
        if (strcmp(args[0], builtins[i]) == 0) {
            return (*builtin_func[i])(args);
        }
    }

    return rsh_external_execute(args);
}

/*
 * Read a line from stdin and return it. Uses getline() for simplicity, it was added
 * to POSIX recently and was originally a GNU extension to the C library.
 */
char *rsh_readline(void)
{
    char *lineptr = NULL;
    size_t n = 0;

    if (getline(&lineptr, &n, stdin) == -1) {
        fprintf(stderr, "rsh: input error\n");
        return NULL;
    } else {
        return lineptr;
    }
}

char **rsh_tokenise(char *line)
{
    const int bufsize_orig = 1;
    int bufsize = bufsize_orig;
    const char *tok_delim = " \t\r\n\a";
    int position = 0;
    char **tokens = malloc(bufsize * sizeof(char*));
    char *token;

    if (!tokens) {
        fprintf(stderr, "rsh: allocation error\n");
        exit(EXIT_FAILURE);
    }

    token = strtok(line, tok_delim);
    while (token != NULL) {
        tokens[position] = token;
        position++;

        if (position >= bufsize) {
            bufsize += bufsize_orig;
            tokens = realloc(tokens, bufsize * sizeof(char*));

            if (!tokens) {
                fprintf(stderr, "rsh: allocation error\n");
                exit(EXIT_FAILURE);
            }
        }

        token = strtok(NULL, tok_delim);
    }

    tokens[position] = NULL;
    return tokens;
}

/*
 * The main function just drops into an infinite loop of Read, Parse, Execute.
 * Read = use getline() to get a line from stdin,
 * Parse = tokenise with strtok(),
 * Execute = use execvp() to execute programs or execute builtin commands.
 */
int main(void)
{
    char *line;
    char **args;

    /*
     * This is the infinite loop of the shell.
     */
    for (;;) {
        /* Check UID and print the correct prompt out */
        if (geteuid() == 0) {
            printf("rsh # ");
        } else {
            printf("rsh $ ");
        }

        line = rsh_readline();
        args = rsh_tokenise(line);
        rsh_execute(args);

        /* Free up the unused memory */
        free(line);
        free(args);
    }
}

Any improvements would be welcome, especially stuff about buffer overflows and memory leaks because I didn't check for many of those.

\$\endgroup\$
0

2 Answers 2

7
\$\begingroup\$

Realloc strategy

I'm not a fan of how you are reallocing your token array to be one element bigger on each new token. That turns into an \$O(N^2)\$ task. I would recommend either:

  1. Do one quick pass over the input to count the number of tokens.
  2. Doubling the size of the token array each time.

Another thing you could do is to reuse the token array and not free it every time.

DIM macro

You have a function rsh_num_builtins(), which I would normally handle with a DIM macro:

#define DIM(array) (sizeof(array)/sizeof(array[0]))

for (i = 0; i < DIM(builtins); i++) {

Error handling

All of your functions return error values or NULL, but main never checks if line is NULL, or uses the return value of rsh_execute().

If you don't actually care about rsh_execute()'s return value, it might be more clear to just change that function to return void. However it seems like in your case you might later extend the shell to use the return value, so it's probably ok the way you have it.

Well written

Your code is written clearly with good comments. I had no trouble reading and understanding it. So good job on that.

\$\endgroup\$
6
\$\begingroup\$

First off rsh is a rather unfortunate name since there already was a tool with this name and it was rather widely known. I realise this is a toy and can have arbitrarily bad name, but it still should not clash with something known.

Secondly:

rsh.c: In function ‘rsh_exit’:
rsh.c:42:21: warning: unused parameter ‘args’ [-Wunused-parameter]
 int rsh_exit(char **args)
                     ^
rsh.c: In function ‘rsh_help’:
rsh.c:67:21: warning: unused parameter ‘args’ [-Wunused-parameter]
 int rsh_help(char **args)
                     ^
rsh.c: In function ‘rsh_external_execute’:
rsh.c:93:11: warning: variable ‘wpid’ set but not used [-Wunused-but-set-variable]
     pid_t wpid;

What's up with that?

Finally, pretty much all comments in the code were unnecessary at best and actively harmful at worst. The more comments you get, the more likely it is they will get out of sync with the code. Only comment when stuff is not obvious.

#include <stdio.h>  /* printf, fprintf */
#include <stdlib.h> /* exit */
#include <string.h> /* strtok */
#include <unistd.h> /* fork, execvp */
#include <sys/wait.h>   /* waitpid */
#include <sys/types.h>  /* waitpid */

sys/ headers should got first.

Such comments quickly get out of date. In fact are already out of date - you use malloc and realloc, which require stdlib.h. I don't see them mentioned.

/* A list of builtin functions here */
char *builtins[] = {
    "exit",
    "cd",
    "help"
};

Useless comment. Such lists typically end up with a NULL pointer. Consider 'static const' qualifiers.

/*
 * Return the number of builtin commands so we can avoid hard-coding random magic
 * numbers and other annoyances everywhere.
 */
int rsh_num_builtins(void)
{
    return sizeof(builtins) / sizeof(char *);
}

This function, as all others (except for main) should be static.

This particular function should not exist in the first place. As was pointed out by someone else the standard way of doing this is a macro. Other approach is having aforementioned NULL at the end of the table and iterating it with a pointer. Assuming macro route, this is still bad. The idiom is to use array[0] element: sizeof(builtins) / sizeof(builtins[0]);. Even though your char * works here, there is no reason to deviate from the idiom.

/*
 * This builtin function changes the directory of the shell process, like the cd
 * in normal shells.
 */
int rsh_cd(char **args)
{   
    if (args[1] == NULL) {
       fprintf(stderr, "rsh: expected argument to cd\n");

You should have a dedicated function which reports errors. In particular it would know to print to stderr and would append 'rsh: ' on its own.

You ignore errno and in effect don't give any reason for the failure.

What happens if there are more arguments?

       return 0;

The standard error would return 1, so this is reversed.

    } else if (chdir(args[1]) == -1) {
       fprintf(stderr, "rsh: can't cd to %s\n", args[1]);
       return 1;
    }

There is no point doing else if and it makes the code slightly less readable. Here you return 1 on failure.

    return 0;

.... but are back to returning 0 on success, which is the same error you return when there are no arguments.

}

/*
 * Print out a help message about rsh
 */
int rsh_help(char **args)
{
    puts("rsh: recovery shell");
    puts("");
    puts("rsh is a very small and simple shell that is used for system recovery purposes.");
    puts("It is not compliant with anything and does not allow escaping, quoting, sourcing");
    puts("backslashing or anything else. It is scriptable, but only in such a way that lets");
    puts("you run commands with simple options and arguments: nothing else!");
    puts("");
    puts("List of builtin commands:");
    puts("exit cd help");

This should iterate over the table and print names. Otherwise it is bound to get out of sync.

    puts("All those builtins do exactly what you expect them to do, so no documentation is");
    puts("needed, hopefully.");

    return 0;
}

int (*builtin_func[]) (char **) = {
    &rsh_exit,
    &rsh_cd,
    &rsh_help
};

So the actual list of functions implementing builtins is detached from builtins list? This is bound to cause problems. The standard way is to obviously have a table of a struct. The struct would contain the name, the pointer and some optional stuff like expect number of arguments.

int rsh_external_execute(char **args)
{
    pid_t pid;
    pid_t wpid;
    int status;

    /* fork off a new process */
    pid = fork();

    if (pid == 0) {
        /* in the child process. Note that this does not exit the parent
         * in an error, with the exit() call. */
        if (execvp(args[0], args) == -1) {
            fprintf(stderr, "rsh: cannot exec\n");
            exit(EXIT_FAILURE);
        }

No point checking the error. If execvp returned, it failed. This should be _Exit.

    } else if (pid < 0) {

The error check should go first for readability.

        /* error forking */
        fprintf(stderr, "rsh: cannot fork\n");
        return -1;
    } else {
        /* parent process */
        do {
            wpid = waitpid(pid, &status, WUNTRACED);
        } while (!WIFEXITED(status) && !WIFSIGNALED(status));

Incorrect. waitpid can return due to an error (e.g. EINTR) in which case 'status' value is inderminate.

    }

    return 0;
}

[parsing code omitted, not in the mood]

/*
 * The main function just drops into an infinite loop of Read, Parse, Execute.
 * Read = use getline() to get a line from stdin,
 * Parse = tokenise with strtok(),
 * Execute = use execvp() to execute programs or execute builtin commands.
 */

Not only useless, but also bad comment. main has 0 business knowing about getline, strtok or execvp.

int main(void)
{
    char *line;
    char **args;

    /*
     * This is the infinite loop of the shell.
     */
    for (;;) {
        /* Check UID and print the correct prompt out */
        if (geteuid() == 0) {

UID != EUID

            printf("rsh # ");
        } else {
            printf("rsh $ ");
        }

Why would you do this on each loop? Determine the right prompt beforehand and only print it when needed.

        line = rsh_readline();
        args = rsh_tokenise(line);
        rsh_execute(args);

        /* Free up the unused memory */
        free(line);
        free(args);
    }
}

Verdict: E-.

\$\endgroup\$

Your Answer

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