4
\$\begingroup\$

I have written a basic C Shell. It features the builtins cd, programmer, ver and exit. The code for the shell, named vsh, is as follows:

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

#define BUFSIZE 1024
#define TOK_BUFSIZE 64
#define TOK_DELIM " \t\r\n\a"

char **split_line(char *line)
{
    int bufsize = TOK_BUFSIZE, position = 0;
    char **tokens = malloc(bufsize * sizeof(char*));
    char *token, **tokens_backup;

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

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

        if (position >= bufsize) {
            bufsize += TOK_BUFSIZE;
            tokens_backup = tokens;
            tokens = realloc(tokens, bufsize * sizeof(char*));
            if (!tokens) {
                free(tokens_backup);
                fprintf(stderr, "vsh: allocation error\n");
                exit(EXIT_FAILURE);
            }
        }

        token = strtok(NULL, TOK_DELIM);
    }
    tokens[position] = NULL;

    return tokens;
}

int main(int argc, char **argv)
{
    int finish = 0;
    char *user = getenv("USER");
    char hostname[BUFSIZE];
    char *cwd;
    char line[BUFSIZE] = {0};
    char **args;
    char *directory;

    printf("Welcome to vsh\n");

    system("/bin/cat /etc/motd");

    printf("\n");

    gethostname(hostname, BUFSIZE);

    while (!finish)
    {
        cwd = getcwd(cwd, BUFSIZE);

        printf("[");
        printf(user);
        printf("@");
        printf(hostname);
        printf("] ");
        printf(cwd);
        printf(" $ ");
        fflush(stdout);

        if(NULL == fgets(line, sizeof line, stdin))
        {
            finish = 1;
            printf("\nLeaving vsh\n");
        }
        else
        {
            printf("The command read was %s", line);
            printf("\n");

            args = split_line(line);

            if(strcmp(line, "") == 0) {
                fprintf(stderr, "vsh: Expected command\n");
            }
            else if(strcmp(args[0], "cd") == 0)
            {
                directory = args[1];
                chdir(directory);
            }
            else if(strcmp(line,"programmer") == 0)
            {
                printf("The programmer of vsh is George Gibson\n");
            }
            else if(strcmp(line,"ver") == 0)
            {
                printf("The current version is 1.0\n");
            }
            else if(strcmp(line,"exit") == 0)
            {
                finish = 1;
                printf("\nLeaving vsh...\n");
                getchar();
            }
            else
            {
                pid_t pid;
                int err;

                pid = fork();

                if(pid == -1) {
                    perror("vsh");
                }
                else if(pid == 0) {
                    execvp(args[0], args);
                    perror("vsh");
                }
                else {
                    waitpid(pid, &err, 0);
                }
            }
        }
    }

    return 0;
}

I'm particularly looking for reviews relating to bugs, optimizations and general improvements.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ You don't need to do so many printf calls. You should use the %s more when printing, especially with user input (hostname, username) from the environment. \$\endgroup\$
    – Lucas Holt
    Aug 8, 2015 at 8:38

2 Answers 2

3
\$\begingroup\$

In your main() function there is a while loop (while (!finish)) and in the first few lines of that there are a lot of printf() calls. Replace all of those printf's with this one line:

printf("[%s@%s] %s $ ", user, hostname, cwd);

It is easier to read that way, and with non optimising compilers is slightly more efficient.

Additionally, your main() function contains a lot of strings with vsh in them. I recommend you replace this:

printf("vsh: allocation error\n");

with this:

printf("%s: allocation error\n", argv[0]);

That will make sure that you can more easily change the name of your shell if you want to in the future. In functions other than main() the bother of passing argv[0] may be more than it is worth, though.

\$\endgroup\$
2
  • 3
    \$\begingroup\$ A single printf with format string isn't just faster or easier to read, the original code with printf(user) is a potential security threat. Think of calling USER="%d" vsh \$\endgroup\$ Aug 8, 2015 at 9:47
  • 1
    \$\begingroup\$ Exactly. With a carefully crafted string one might be able to execute arbitrary code. \$\endgroup\$ Aug 8, 2015 at 9:59
2
\$\begingroup\$
  • You seem to inconsistently use conditional checks:

    • The ! operator:

      if (!tokens) {
      
    • "Yoda conditions":

      if(NULL == fgets(line, sizeof line, stdin))
      
    • Not-Yoda conditions:

      if(strcmp(line, "") == 0) {
      

    The second one can still use the ! operator (in any case with NULL), though not the third one because strcmp() returns an int. In any case, you should stick to one style for consistency and where applicable. It's one's own preference, so choose whichever you prefer.

  • This doesn't look too ideal:

    if(NULL == fgets(line, sizeof line, stdin))
    {
        finish = 1;
        printf("\nLeaving vsh\n");
    }
    

    Consider making the while loop infinite by using 1 or true instead of finish. At first glance, it may not be clear to the reader that the program should end after this point since there's no return or exit(). This will help make it easier for the reader to identify all termination points of the program as opposed to scoping the entire loop flow.

  • It may be necessary to terminate the program if either fork() or execvp() fails. That is usually more common, but it may still depend on how failure will affect the rest of the program.

\$\endgroup\$

Your Answer

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