7
\$\begingroup\$

The M-Shell (msh) provides a basic command-line interface similar, and features some builtins (cd, exit, help, whoami, kill).

#ifdef _POSIX_C_SOURCE
#undef _POSIX_C_SOURCE
#endif

#ifdef _XOPEN_SOURCE
#undef _XOPEN_SOURCE
#endif

#define _POSIX_C_SOURCE 200819L
#define _XOPEN_SOURCE 700

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>
#include <stdint.h>
#include <ctype.h>

#include <unistd.h>
#include <sys/wait.h>
#include <errno.h>
#include <pwd.h>
#include <libgen.h>

#define ARRAY_CARDINALITY(x) (sizeof (x) / sizeof ((x)[0]))

#define MSH_TOK_DELIM   " \t\r\n\v\f"

static int msh_cd(const char *const *argv);
static int msh_help(const char *const *argv);
static int msh_exit(const char *const *argv);
static int msh_kill(const char *const *argv);
static int msh_whoami(const char *const *argv);

/* 
*   List of builtin commands, followed by their corresponding functions. 
*/
static struct {
    const char *const builtin_str;
    int (* const builtin_func)(const char *const *);
} const builtin[] = {
    { "cd", &msh_cd },
    { "help", &msh_help },
    { "exit", &msh_exit },
    { "kill", &msh_kill },
    { "whoami", &msh_whoami },
};

static int msh_whoami(const char *const *argv)
{
    const uid_t uid = geteuid();
    const struct passwd *const pw = (errno = 0, getpwuid(uid));

    if (argv[1]) {
        fputs("-msh: extra operand to \"whoami\".\n", stderr);
    } else if (!pw || errno) {
        perror("-msh: ");
    } else {
        puts(pw->pw_name);
    }
    return 1;
}

static int msh_kill(const char *const *argv)
{
    if (!argv[1] || !argv[2]) {
        fputs("-msh: expected argument to \"kill\".\n", stderr);
    } else
        if (kill((pid_t) strtol(argv[2], 0, 10), (int) strtol(argv[1], 0, 10)) == -1) {
        perror("-msh ");
    }
    return 1;
}

static int msh_cd(const char *const *argv)
{
    if (!argv[1]) {
        fputs("-msh: expected argument to \"cd\".\n", stderr);
    } else if (chdir(argv[1])) {
        perror("-msh");
    }
    return 1;
}

static int msh_help(const char *const *argv)
{
    (void) argv;
    puts("Z-Shell\n"
         "Type program names and arguments, and hit enter.\n"
         "The following are built-in:\n");

    for (size_t i = 0; i < ARRAY_CARDINALITY(builtin); ++i) {
        puts(builtin[i].builtin_str);
    }

    puts("Use the man command for information on other programs.\n");
    return 1;
}

static int msh_exit(const char *const *argv)
{
    (void) argv;
    return 0;
}

/* Calls fork and execvp to duplicate and replace a process, returns 0 on failure and 1 on success
*/
static int msh_launch(const char *const *argv)
{
    int status = 0;
    int pid = fork();
    
    if (!pid) {
        if (execvp(argv[0], (char *const *) argv) == -1) {
            perror("msh: ");
            return 0;
        }
    }

    if (pid == -1) {
        perror("msh: ");
        return 0;
    }

    do {
        waitpid(pid, &status, WUNTRACED);
    } while (!WIFEXITED(status) && !WIFSIGNALED(status));

    return 1;
}

/* Returns 1 in the absence of commands or a pointer to a function if argv[0] was a built-in command.
*/
static int msh_execute(const char *const *argv)
{
    if (!argv[0]) {
        /* No commands were entered. */
        return 1;
    }

    for (size_t i = 0; i < ARRAY_CARDINALITY(builtin); ++i) {
        if (!strcmp(argv[0], builtin[i].builtin_str)) {
            return (*builtin[i].builtin_func) (argv);
        }
    }
    return msh_launch(argv);
}

/* Returns a char pointer on success, or a null pointer on failure.
*  Caller must free the line on success.
*  Otherwise, msh_read_line frees all allocations and set them to point to NULL.
*/
static char *msh_read_line(int *err_code)
{
    const size_t page_size = BUFSIZ;
    size_t position = 0;
    size_t size = 0;
    char *content = 0;

    clearerr(stdin);

    for (;;) {
        if (position >= size) {
            size += page_size;
            char *new = realloc(content, size);

            if (!new) {
                *err_code = ENOMEM;
                return 0;
            }
            content = new;
        }
        int c = getc(stdin);

        if (c == EOF || c == '\n') {
            if (feof(stdin)) {
                free(content);
                *err_code = EOF;
                return 0;
            } else {
                content[position] = '\0';
                return content;
            }
        } else {
            content[position] = (char) c;
        }
        position++;
    }
}

/* Returns a pointer to pointers to null-terminated strings, or a NULL pointer on failure. 
*  Does not free the passed char * in case of failure. 
*/
static char **msh_parse_args(char *line)
{
    const size_t page_size = 128;
    size_t position = 0;
    size_t size = 0;
    char **tokens = 0;

    for (char *next = line; (next = strtok(next, MSH_TOK_DELIM)); next = 0) {
        if (position >= size) {
            size += page_size;
            char **tmp = realloc(tokens, size * sizeof *tmp);

            if (!tmp) {
                free(tokens);
                return 0;
            }
            tokens = tmp;
        }
        tokens[position++] = next;
    }

    if (tokens) {
        tokens[position] = 0;
    }
    return tokens;
}

static int msh_loop(void)
{
    int status = 0;

    do {
        char *line = NULL;
        char **args = NULL;
       
        /* getuid() is always successful. */
        const uid_t uid = getuid();
        const struct passwd *const pw = getpwuid(uid);
    
        char *cwd = getcwd(0, 0);
        char *base_name = cwd ? basename(cwd) : 0;
        
        printf("%s:~/%s $ ", pw ? pw->pw_name : "", base_name? base_name : "");

        int err_code = 0;

        line = msh_read_line(&err_code);
        if (!line) {
            if (err_code == ENOMEM) {
                perror("-msh ");
            }
            fputc('\n', stdout);
            free(cwd);
            return 0;
        }
    
        if (!*line) {
            continue;
        }

        if (!(args = msh_parse_args(line))) {
            perror("-msh ");
            free(line);
            free(cwd);
            return 0;
        }

        status = msh_execute((const char *const *) args);
        free(line);
        free(args);
        free(cwd);
    } while (status);
    
    return 1;
}

int main(void)
{
    return !msh_loop() ? EXIT_FAILURE : EXIT_SUCCESS;
}

The final executable is 14552 bytes, does not have a large memory footprint:

3442:   ./msh
Address           Kbytes     RSS   Dirty Mode  Mapping
0000564a55b19000       4       4       0 r---- msh
0000564a55b1a000       4       4       0 r-x-- msh
0000564a55b1b000       4       4       0 r---- msh
0000564a55b1c000       4       4       4 r---- msh
0000564a55b1d000       4       4       4 rw--- msh
0000564a579fb000     132      16      16 rw---   [ anon ]
00007f1e932fa000      12       8       8 rw---   [ anon ]
00007f1e932fd000     160     160       0 r---- libc.so.6
00007f1e93325000    1620    1004       0 r-x-- libc.so.6
00007f1e934ba000     352     216       0 r---- libc.so.6
00007f1e93512000       4       0       0 ----- libc.so.6
00007f1e93513000      16      16      16 r---- libc.so.6
00007f1e93517000       8       8       8 rw--- libc.so.6
00007f1e93519000      52      24      24 rw---   [ anon ]
00007f1e93538000       8       4       4 rw---   [ anon ]
00007f1e9353a000       8       8       0 r---- ld-linux-x86-64.so.2
00007f1e9353c000     168     168       0 r-x-- ld-linux-x86-64.so.2
00007f1e93566000      44      44       0 r---- ld-linux-x86-64.so.2
00007f1e93572000       8       8       8 r---- ld-linux-x86-64.so.2
00007f1e93574000       8       8       8 rw--- ld-linux-x86-64.so.2
00007ffd60554000     132      12      12 rw---   [ stack ]
00007ffd605b7000      16       0       0 r----   [ anon ]
00007ffd605bb000       8       4       0 r-x--   [ anon ]
ffffffffff600000       4       0       0 --x--   [ anon ]
---------------- ------- ------- ------- 
total kB            2780    1728     112

and unlike other shells, reports no memory leaks under valgrind.

Review Goals:

Is the code structured properly?

General coding comments, style, etc.

Does any part of the code invoke undefined/implementation-defined behavior?

\$\endgroup\$
8
  • \$\begingroup\$ Clearly this shell is less capable than bash, which can do | pipes, and that's cool. It would be interesting to see some "SoTA comparisons" in the review context, to motivate this exercise. For example, the ash and dash shells were crafted with an explicit design goal of minimalism. The idea was to place a tiny shell interpreter on a small /boot partition, and to consume very little memory while /etc/rc and other bootup scripts are running. "Small" can mean "secure", fewer bugs. Perhaps there are some nice "small footprint" stats you'd like to highlight in this question? \$\endgroup\$
    – J_H
    Jan 25 at 18:33
  • \$\begingroup\$ @J_H Well, would the result of pmap be of any interest? \$\endgroup\$
    – Harith
    Jan 25 at 19:31
  • \$\begingroup\$ Sure. It’s often the case that a given program is best at “something”. I’m just looking for what that might be in this case. \$\endgroup\$
    – J_H
    Jan 25 at 19:34
  • \$\begingroup\$ Have you tested thecd functionality and is it working as expected? Does it work repeatedly? \$\endgroup\$
    – pacmaninbw
    Jan 27 at 19:30
  • \$\begingroup\$ @pacmaninbw Yes, it works as expected, and works repeatedly. Do you see any potential bug? (Or are you talking about cd ~/cd -/cd? No, it doesn't recognize them.) \$\endgroup\$
    – Harith
    Jan 28 at 8:57

2 Answers 2

3
\$\begingroup\$

Good job! It definitely lives up to its name of "minimal shell". And while a small codebase doesn't always translate to a bug-free codebase, there's certainly fewer places for bugs to hide.

I really like the layering. It makes a lot of sense as you read the source.

name

static int msh_help( ... )
{
    ...
    puts("Z-Shell\n"

Z ?!?

I was kind expecting to see "msh" there.

automated tests

This submission would benefit from adding a test suite that exercises the target code.

NULL

https://linux.die.net/man/3/getcwd

As an extension to the POSIX.1-2001 standard, Linux (libc4, libc5, glibc) getcwd() allocates the buffer dynamically using malloc(3) if buf is NULL.

        char *cwd = getcwd(0, 0);

Call me old fashioned, but I would rather see getcwd(NULL, 0) there. (Yes, yes, I know, zero and NULL are nearly the same.)

check error status

Thank you for helpfully pointing out that getuid() always succeeds.

But some of the calls you make can fail, e.g. with ENOMEM on an unlucky call to getcwd().

I am very glad to see you carefully calling free().

goto outN

        line = msh_read_line( ... );
        if (!line) {
            ...
            free(cwd);

I feel that, instead of freeing cwd, this should be goto out3;.

        if (!( ... msh_parse_args(line))) {
            ...
            free(line);
            free(cwd);

Similarly, goto out2;.

And out1: would free line.

    out2:
        free(args);
    out3:
        free(cwd);

        return ret_val;

We would need to properly set up the return value.

The idea here is, for each allocate call, we write exactly one deallocate call, nicely paired up.

Following such a pattern should make it harder to accidentally introduce code defects.

allocation lifetime

*  Does not free the passed char * in case of failure. 
*/
static char **msh_parse_args(char *line)
{
            ...
            if ( ... ) {
                free(tokens);

I started writing this review remark thinking I would ask for caller to always be responsible for free'ing line.

But then I realized the comment was misleading me. I imagine it used to be true, at one time.

lint

static int msh_exit(const char *const *argv)
{
    (void) argv;

I assume you wrote this to silence a -Wall -pedantic type of warning, and that's great, I support such practice.

IDK, maybe assert that there's no arguments supplied? Which should keep the compiler happy, since we actually used the input parameter.

boolean

static int msh_execute( ... )

Rather than int, I feel this is more naturally a bool. Though the sense is negated.

OIC, launch can return arbitrary values, so maybe introduce a status type? Similar to EXIT_{FAILURE,SUCCESS} ?

I keep finding it harder than necessary to read things like this:

static int msh_launch( ... )
            ...
            return 0;

because it doesn't say something like return EXIT_SUCCESS.

I do like how you DRYed up help by looping and displaying each .builtin_str.

The whoami and similar routines are wonderfully simple and robust.


This codebase tackles modest goals, and accomplishes them, within a small memory footprint.

I would be willing to delegate or accept maintenance tasks on it.


EDIT for @pacmaninbw

(As far as "teaching" goes, I would rather teach folks to choose a high level language instead of portable assembly. Then we do a better job of conveying technical ideas to other humans. But hey, C is what it is, sometimes ya gotta use it, so I roll with the punches.)

Assume "mutex acquisition failed" is represented by a NULL return value.

Here is the usual (carefully structured, nested) pattern:

void do_stuff() {
    int ret = EXIT_FAILURE;

    if ((a = acquire_mutex("a")) == NULL) {
        goto out_a;
    }
    if ((b = acquire_mutex("b")) == NULL) {
        goto out_b;
    }
    if ((c = acquire_mutex("c")) == NULL) {
        goto out_c;
    }
    do_cool_stuff_while_holding(a, b, c);
    ret = EXIT_SUCCESS;

    unlock("c");
  out_c:
    unlock("b");
  out_b:
    unlock("a");
  out_a:
    return ret;
}

Notice that we obey lexical ordering of lock names, and carefully release them in reverse order, to prevent ugly scenarios like deadlock.

It doesn't matter what the resource is. Could be a lock, a malloc'd string, a file descriptor, whatever. The routine needs them in order to accomplish its contract. It acquires them in sequence. But sometimes things don't go smoothly. So it releases them in reverse sequence when it bails out.

If we don't follow this pattern, we may wind up replacing the if ... goto clauses with copy-pasta code. First if ... "release A, then bail". Second if ... "release B, A, then bail". (Wait, did I carefully release in the proper order?) Third if ... "release C, B, A, then bail".

Now someone refactors B, turning it into D. And correctly fixes 50% of the places where we free it. Sigh! Or someone messes up the unlock order.

Remember this is the exceptional path we're considering here, not the happy path. So test coverage is going to be challenging, and spotty.

Forcing every single release to always execute in the Happy Path is a boon to testing. So jumping into the middle of the "release" flow is a big win here.

Cultural digression: An engineer, perhaps Edsger Dijkstra or Niklaus Wirth, could nest structured elements so that a,b,c nest nicely, and so does the unwinding code for releasing resources. But hey, Linus don't code that way. So we roll with it, we don't write a bunch of trivial helpers, and we don't see the bulk of our logic inching further and further away from the left margin.

Here is a typical example in BIND9, that is, in some non-kernel code:

00331 static inline dns_rdatalist_t *
00332 newrdatalist(dns_message_t *msg) {
00337         if ( ... ) { ...
00339                 goto out;
00340         } ...
00355  out:
00356         if ( ... )
00357                 dns_rdatalist_init(rdatalist);
00358 
00359         return (rdatalist);
00360 }
\$\endgroup\$
3
  • \$\begingroup\$ I was going to up vote until I saw you suggest goto, why do you want to teach spaghetti code? \$\endgroup\$
    – pacmaninbw
    Jan 29 at 2:51
  • 4
    \$\begingroup\$ It's a standard idiom in C code, e.g. in the linux kernel we see it ALL the time. And no, it's not spaghetti, it is structured. It's a pattern that is easy for humans to follow and causes fallible humans to write fewer code defects. // In a higher level language like lisp or python, we might throw an exception, confident that the Right Thing will happen and resources will be cleaned up on the way out. Or roughly equivalently, use a context manager. In C we do not enjoy that luxury. So we pair allocate with deallocate. Some form of goto fail, goto out, or goto outN accomplishes cleanup. \$\endgroup\$
    – J_H
    Jan 29 at 2:56
  • 2
    \$\begingroup\$ Yes, this is the single widely-accepted use of goto in C. Using that pattern would likely have avoided the memory leak I mentioned in my answer. \$\endgroup\$ Jan 30 at 17:03
1
\$\begingroup\$

The #ifdef here adds no value:

#ifdef _POSIX_C_SOURCE
#undef _POSIX_C_SOURCE
#endif

It's perfectly permissible and normal to #undef names whether or not they are defined.


I don't like the name hard-coded here and elsewhere:

    perror("-msh: ");

It's much better to store the name from argv[0] to match how the user accessed the program (which may have a leading - if it's invoked from login). Also, the extra : and space look weird alongside those that perror() produces.


Here's a memory leak:

    if (!*line) {
        continue;
    }

We need to free line and cwd before we continue and those variables end their scope.

It's quite easy to trigger this leak - here's what happens when I send just two newlines as input:

==1310940== HEAP SUMMARY:
==1310940==     in use at exit: 8,223 bytes in 2 blocks
==1310940==   total heap usage: 22 allocs, 20 frees, 30,928 bytes allocated
==1310940== 
==1310940== 31 bytes in 1 blocks are definitely lost in loss record 1 of 2
==1310940==    at 0x4845C43: realloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1310940==    by 0x496F9E7: getcwd (getcwd.c:86)
==1310940==    by 0x109869: msh_loop (289119.c:233)
==1310940==    by 0x1099CF: main (289119.c:272)
==1310940== 
==1310940== 8,192 bytes in 1 blocks are definitely lost in loss record 2 of 2
==1310940==    at 0x4840718: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1310940==    by 0x10967B: msh_read_line (289119.c:165)
==1310940==    by 0x1098E0: msh_loop (289119.c:240)
==1310940==    by 0x1099CF: main (289119.c:272)
\$\endgroup\$
2
  • \$\begingroup\$ Just when I thought I was not leaking any memory. :) \$\endgroup\$
    – Harith
    Jan 30 at 18:48
  • \$\begingroup\$ If it helps, it was gcc -fanalyzer that alerted me to the leak. That's becoming a more useful option with each successive GCC version. \$\endgroup\$ Feb 11 at 13:00

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.