10
\$\begingroup\$

I was given an assignment to write a mini-shell:

To write your own shell, you will need to start with a C program that will prompt the user for input and accept in a number of arguments from a command line. The commands entered will be accepted into your shell and then processed to understand if it is a built in command or something that needs to be executed via fork/exec (NOTE: No use of system function). Your shell should emulate the standard shells in how it deals with background commands ( & ). The interaction with your shell should be just like the standard shells. What I mean by this is to have a good usage statement returned if the arguments passed to the shell are not correct, and when there is an error you should send back a useful error, and not exit the shell, just continue.

Namespaces allow for virtualization and sharing of spaces between parent and child processes. This is a part of the Linux operating system since 2008 that allows for the creation of different models to create containers for software applications. The most popular version of Linux namespaces is Docker. Your task for this lab is to create the option inside your shell through “built in” commands to move your shell into a container. The options for different containers can be added together in a clone or clone2 call. Here are some of the options:

  • CLONE_NEWIPC - New namespace for IPC
  • CLONE_NEWUTS - Setup new hostname and domain
  • CLONE_NEWUSER - User and group changes
  • CLONE_NEWNET - New network namespace
  • CLONE_NEWNS - New mount namespace

When using a clone function, you will have the ability to run another function. To test your clone call, you will need to be able to demonstrate the change, and the best way to do that is to spawn another shell to “look around” at what changed. The best way to do this is to spawn another shell using the system function call.

Here is my code implementing the above. Basic testing has been performed and it seems to work as intended:

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

#define _GNU_SOURCE
#include <linux/sched.h>

#ifndef streq
#define streq(x, y) (strcmp((x), (y)) == 0)
#endif

#define MAX_COMMAND_LENGTH 100
#define MAX_PARAMS 10

typedef struct input
{
    char** params;
    int numParams;
    bool inBackground;
} Command;

// split input into array of parameters
Command* parseCmd(char* input)
{
    // handle bad input
    if (!input || strlen(input) == 0)
        return NULL;

    // allocate space on heap for command struct, and parameter list
    Command *cmd = calloc(1, sizeof(Command));
    cmd->params = calloc(MAX_PARAMS + 1, sizeof(char*));

    // setup strtok_r
    char *next = NULL;
    char *part = strtok_r(input, " ", &next);

    // do-while so we can process first `part`, then rest of `input`
    do
    {
        // check for background, if not found default setting is false
        if (streq(part, "&"))
        {
            cmd->inBackground = true;

            // don't store in parameter list for commands
            continue;
        }

        cmd->params[cmd->numParams] = part;
        ++cmd->numParams;
    }
    while ((part = strtok_r(NULL, " ", &next)));

    return cmd;
}

void freeCmd(Command* cmd)
{
    free(cmd->params);
    cmd->params = NULL;

    free(cmd);
    cmd = NULL;
}

int childFunct(void)
{
    system("sh");
    return 0;
}

int executeClone(Command *cmd)
{

    const int STACK_SIZE = 1024;
    unsigned long flags = 0;

    if (cmd->numParams <= 1)
    {
        fprintf(stderr, "clone command requires arguments\n");
        return 0; // non-fatal error
    }

    if (streq(cmd->params[1], "ipc"))
        flags |= CLONE_NEWIPC;
    if (streq(cmd->params[1], "uts"))
        flags |= CLONE_NEWUTS;
    if (streq(cmd->params[1], "user"))
        flags |= CLONE_NEWUSER;
    if (streq(cmd->params[1], "net"))
        flags |= CLONE_NEWNET;
    if (streq(cmd->params[1], "ns"))
        flags |= CLONE_NEWNS;
    if (!flags)
    {
        fprintf(stderr, "clone parameter not recognized\n");
        return 0;  // non-fatal error
    }

    char *stack = malloc(STACK_SIZE);
    pid_t pid = clone(childFunct, stack + STACK_SIZE, flags, NULL);

    // error
    if (pid == -1)
    {
        char* error = strerror(errno);
        fprintf(stderr, "clone: %s\n", error);
    }

    return 0;
}

int executeCmd(Command *cmd)
{
    pid_t pid = fork();

    // error
    if (pid == -1)
    {
        char* error = strerror(errno);
        fprintf(stderr, "fork: %s\n", error);
        return 1;
    }

    // child process
    else if (pid == 0)
    {
        // will daemonize child if background
        // if (cmd->inBackground)
        //     setpgid(pid, 0);

        // execute command
        execvp(cmd->params[0], cmd->params);

        // error occurred
        char* error = strerror(errno);
        fprintf(stderr, "ERROR: %s: %s\n", cmd->params[0], error);
        return 2;
    }

    // parent process
    else
    {
        // wait for child process to finish
        if (!cmd->inBackground)
        {
            int childStatus = 0;
            waitpid(pid, &childStatus, 0);
        }
    }

    return 0;
}

int main(void)
{
    char input[MAX_COMMAND_LENGTH + 1] = "";
    Command *cmd = NULL;

    while(1)
    {
        // print command prompt
        fputs("> ", stdout);

        // fgets returns NULL on Ctrl+D, so exit the loop on NULL
        if (!fgets(input, sizeof(input), stdin))
            break;

        // remove trailing newline character, if any
        if (input[strlen(input) - 1] == '\n')
            input[strlen(input) - 1] = '\0';

        // split cmd into array of parameters
        cmd = parseCmd(input);

        // handle bad command, continue running
        if (!cmd)
            continue;

        // exit?
        if (streq(cmd->params[0], "exit") || streq(cmd->params[0], "quit"))
            break;

        // test for built-in command
        if (streq(cmd->params[0], "clone"))
        {
            if (executeClone(cmd))
            break;
        }
        else // executable
        {
            if (executeCmd(cmd))
                break;
        }

        freeCmd(cmd);
    }
}

Any suggestions for improvement?

\$\endgroup\$
2
+50
\$\begingroup\$
  1. Avoid a hacker exploit. The null character is not special when fgets() reads it. So if the first character read is the rare null character, then input[strlen(input) - 1] is UB. strcspn() is a nice alternative.

    if (!fgets(input, sizeof(input), stdin)) break;
    //if (input[strlen(input) - 1] == '\n')
    //    input[strlen(input) - 1] = '\0';
    input[strcspn(input, "\n")] = '\0';
    
  2. Memory leak in main(). Better to freeCmd(cmd) after the various breaks.

            freeCmd(cmd);
        }
        // add 
        freeCmd(cmd);
    }
    
  3. Unclear why code is using long for flags. linux.die.net implies the type should be int. Check your prototype for clone() - also its return type. Is it int or pid_t Ref?

  4. Although code nicely aligns, I'd expect an if() … else if() …else … structure rather than if() … if() …

    if (streq(cmd->params[1], "ipc"))
        flags |= CLONE_NEWIPC;
    else /* add */ if (streq(cmd->params[1], "uts"))
        flags |= CLONE_NEWUTS;
    else /* add */ if (streq(cmd->params[1], "user"))
        flags |= CLONE_NEWUSER;
    else {
        fprintf(stderr, "clone parameter not recognized\n");
        return 0;  // non-fatal error
    }
    
  5. Why try to synchronize the type? Instead reference the type. Pedantic, avoid UB of signed overflow, use unsigned math with 1u.

    //                                      Is this the right type? 
    // cmd->params = calloc(MAX_PARAMS + 1, sizeof(char*));
    //                                    Right size, regardless of type of *cmd->params
    cmd->params = calloc(MAX_PARAMS + 1u, sizeof *(cmd->params));
    
\$\endgroup\$
  • \$\begingroup\$ Aren't parameters not "stackable" when you change to the if-else construct? \$\endgroup\$ – syb0rg Oct 13 '16 at 5:25
  • \$\begingroup\$ @syb0rg Comment is unclear for me -please rephrase - will check later. \$\endgroup\$ – chux - Reinstate Monica Oct 13 '16 at 5:39
  • 1
    \$\begingroup\$ My bad, I misunderstood a part of your review. \$\endgroup\$ – syb0rg Oct 13 '16 at 17:16
4
\$\begingroup\$
  • Background task handling

    You must wait for the child process regardless of how it is started, in the background or not. As written, a background command ends up in the zombie state. To let the parent shell to continue, set up a signal handler for SIGCHLD, and wait there.

    I know the program statement doesn't require it, but it always nice to let user query a return status of last command.

  • command("sh")

    It is unclear from the program statement, which shell should be cloned. In any case, it is highly recommended to supply a full path to system.

  • freeCmd

    cmd->params = NULL and cmd = NULL are meaningless: cmd = NULL is invisible to caller, and caller shall not touch params of a freed cmd anyway.

  • parseCmd

    The code recognizes & anywhere within a command line as a background indicator. It doesn't feel right for many reasons: compatibility with existing shells; & could be valid character in a filename; etc. I recommend to test for a last argument being & after parsing is done, and adjust the command appropriately.

  • Built-in commands

    Is clone the only built-in? If so, your shell is at least unable to cd.

  • Misc

    • You may want to fflush(stdout) after printing the prompt.

    • Test what malloc returns.

    • It is unclear whether the command line may have quotes and escapes. They are obviously not addressed.

Edit: handling background termination

In a nutshell it seems is fairly simple: define a function

void handle_child_exit(int signo)
{
    int status;
    wait(&status);
    signal(SIGCHLD, handle_child_exit); // Not always required, but helpful
}

and in install it in main:

    signal(SIGCHLD, handle_child_exit);

The reality is a bit hairier, and I can only recommend reading man signal and man sigaction.

\$\endgroup\$
  • \$\begingroup\$ Could you give an example of how you would set up a signal handler for SIGCHLD? \$\endgroup\$ – syb0rg Oct 9 '16 at 22:28
  • \$\begingroup\$ @syb0rg See edit. \$\endgroup\$ – vnp Oct 9 '16 at 22:47
  • \$\begingroup\$ Why not just install signal(SIGCHLD, SIG_IGN) in main and leave what I have? Is there an advantage to this method? \$\endgroup\$ – syb0rg Oct 11 '16 at 22:06
  • \$\begingroup\$ @syb0rg As written, only the foreground command is waited for. On the other hand, you cannot wait on a background command synchronously - it will defeat the purpose of backgrounding - so it is better to do it in a signal handler. \$\endgroup\$ – vnp Oct 11 '16 at 23:04
  • 1
    \$\begingroup\$ Disagree with "cmd->params = NULL and cmd = NULL are meaningless: cmd = NULL is invisible to caller, and caller shall not touch params of a freed cmd anyway." Setting pointers and data that are going to be part of free'd memory, in my experience, more readily exposes mis-use of free'd memory more rapidly than not. A net debugging benefit vs potential incurred problems. cmd = NULL; after free(cmd); is also defensive coding should subsequent code inadvertently use cmd as again, more likely to catch coding problems. Yet agree that it adds no correct code benefit. \$\endgroup\$ – chux - Reinstate Monica Oct 13 '16 at 5:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.