2
\$\begingroup\$

I'm writing a simple shell and want to parse and execute a simple shell script.

PAGER=more
if type less > /dev/null;then PAGER=less; fi
echo $PAGER
printenv|grep $1|$PAGER

The above sets a pager and greps the environment variables for a variable.

I run the script from my shell after I start it:

$ ./shell 
'PATH' is set to /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/usr/local/go/bin.
$ ../checkenv.sh GNOME
20470: executing ../checkenv.sh
less
GNOME_KEYRING_PID=
INSTANCE=GNOME
GNOME_KEYRING_CONTROL=
GNOME_DESKTOP_SESSION_ID=this-is-deprecated
XDG_CURRENT_DESKTOP=GNOME
(END)


20470: executed
$ 

Looks like it's "working" but what do you think of the code?

int main(int argc, char *argv[]) {
    bool donotrun = false;
    struct sigaction new_action, old_action;
    hashtable_t *hashtable = ht_create(65536);
    /* Set up the structure to specify the new action. */
    new_action.sa_handler = termination_handler;
    sigemptyset(&new_action.sa_mask);
    new_action.sa_flags = 0;

    sigaction(SIGINT, NULL, &old_action);
    if (old_action.sa_handler != SIG_IGN)
        sigaction(SIGINT, &new_action, NULL);
    sigaction(SIGHUP, NULL, &old_action);
    if (old_action.sa_handler != SIG_IGN)
        sigaction(SIGHUP, &new_action, NULL);
    sigaction(SIGTERM, NULL, &old_action);
    if (old_action.sa_handler != SIG_IGN)
        sigaction(SIGTERM, &new_action, NULL);

    bool background = false;
    int index = 0;
    int i;
    char *cvalue = NULL;
    const char *commandFile = NULL;

    while (1) {
        index = 0;
        i = getopt_long(argc, argv, "pc:fvh",
                        options, &index);
        if (i == -1)
            break;
        switch (i) {
            case 'p': {
                exit(EXIT_SUCCESS);
            }
            case 'v': {
                printf("sh OpenShell version 0.1(a)\n");
                printf("Version: %s\n", VERSION);

              //  printf ("%s / %s / %s / %s\n",
                //         program_name, version,
                  //       build_date, build_git_sha);


                exit(EXIT_SUCCESS);

            }
            case 'h': {
                usage();
                exit(EXIT_SUCCESS);

            }
            case 'c': {
                cvalue = optarg;
                command(cvalue, hashtable, background);
                exit(EXIT_SUCCESS);
            }

            case 'f': {
                /*
                 * Execute commands from file.
                 * This is used for osh script files.
                 * The quiet flag is also set.
                 */
                //if ((argc != 1) || commandFile)
                    //usage();
                //quietFlag = TRUE;
                printf("case f\n");
                //commandFile = *argv++;

                argc--;
                *argv++;
                *argv++;
                readFile(*argv++, argc, argv, hashtable, background);
                //free(line);
                exit(0);

                //break;
            }

            case '?':
                if (optopt == 'c')
                    fprintf(stderr, "Option -%c requires an argument.\n", optopt);
                else if (isprint (optopt))
                    fprintf(stderr, "Unknown option `-%c'.\n", optopt);
                else
                    fprintf(stderr,
                            "Unknown option character `\\x%x'.\n",
                            optopt);
            default: {
                return 1;
            }
        }
    }
    getPath();
    char *copy = "";

    for (; ;) {
        bool scanning = true;
        while (scanning) {
            char *line = NULL;
            line = readline("$ ");
            if (line == NULL) {
                /* No more lines, so exit the loop. */
                break;
            }
            if (line)
                copy = strdup(line);

            if (line && !strstr(line, "for") && !strstr(line, "==") && !strstr(line, "if") && strstr(line, "=")) {
                donotrun = true;
                char str[128];
                char *ptr;
                strcpy(str, line);
                strtok_r (str, "=", &ptr);
                ht_set(hashtable, str, ptr);
            }

            if (!scanning)
                break;

            if (commandFile!=NULL || !isatty(fileno(stdin))) {
                *argv++;
                readFile(*argv++, argc, argv, hashtable, background);
                free(line);
                exit(0);
            }
            else {
                if (!donotrun) {
                    line = strrep(line, " | ", "|");
                    line = strrep(line, " |", "|");
                    background = testFn2(line);
                    if (background)
                        line[strlen(line) - 1] = '\0';
                    command(line, hashtable, background);
                }
                donotrun = false;
                add_history(copy);

            }
            free(copy);
        }
    }
    //  ParseFree(pParser, free);FIXME: where should this go?
    return 0;
/*
 * Read commands from the specified file.
 * A null name pointer indicates to read from stdin.
 */
static int readFile(const char *name, int argc, char ** argv, hashtable_t *hashtable, bool background) {
    FILE *fp;
    int cc;
    bool ttyFlag;
    char buf[CMD_LEN];
    int r = 0;

    if (sourceCount >= MAX_SOURCE) {
        fprintf(stderr, "Too many source files\n");

        return 1;
    }

    fp = stdin;
    printf("name %s\n", name);

    if (name) {
        fp = fopen(name, "r");

        if (fp == NULL) {
            perror(name);

            return 1;
        }
    }

    sourcefiles[sourceCount++] = fp;

    ttyFlag = isatty(fileno(fp));
    int i = 0;
    while (true) {
        if (ttyFlag)
            showPrompt();

        if (intFlag && !ttyFlag && (fp != stdin)) {
            fclose(fp);
            sourceCount--;

            return 1;
        }
        if (fgets(buf, CMD_LEN - 1, fp) == NULL) {
            if (ferror(fp) && (errno == EINTR)) {
                clearerr(fp);

                continue;
            }

            break;
        }

        cc = strlen(buf);
        if (buf[cc - 1] == '\n')
            cc--;

        while ((cc > 0) && isBlank(buf[cc - 1]))
            cc--;

        buf[cc] = '\0';
        //printf("buf %s\n", argv[0]);
        strreplace(buf, "$1", argv[0]);

        //printf("arg %s\n", ++argv);

        if (strstr(buf, "=")) {
            char str[128];
            char *ptr;
            strcpy(str, buf);
            strtok_r (str, "=", &ptr);
            ht_set(hashtable, str, ptr);
        }

        //printf("the command is %s\n", buf);
        r = command(buf, hashtable, background);
        i++;
    }

    if (ferror(fp)) {
        perror("Reading command line");

        if (fp == stdin)
            exit(1);
    }

    clearerr(fp);

    if (fp != stdin)
        fclose(fp);

    sourceCount--;

    return r;
}


/*
 * Parse and execute one null-terminated command line string.
 * This breaks the command line up into words, checks to see if the
 * command is an alias, and expands wildcards.
 */
int command(char *cmd, hashtable_t *hashtable, bool background) {

    const char *endCmd;
    char cmdName[CMD_LEN];
    freeChunks();
    //printf("cmd %s\n", cmd);
    /*
     * Skip leading blanks.
     */
    if (cmd) {
        while (isBlank(*cmd))
            cmd++;
        /*
         * If the command is empty or is a comment then ignore it.
         */

        if ((*cmd == '\0') || (*cmd == '#'))
            return 0;
        /*
         * Look for the end of the command name and then copy the
         * command name to a buffer so we can null terminate it.
         */
        endCmd = cmd;

        while (*endCmd && !isBlank(*endCmd))
            endCmd++;

        memcpy(cmdName, cmd, endCmd - cmd);
        cmdName[endCmd - cmd] = '\0';
        /*
         * Expand simple environment variables
         */
        while (!strstr(cmd, "((") && strstr(cmd, "$("))
            expandVariable((char *) cmd, hashtable);

        /*
        * Expand POSIX variable expansion e g $ echo $str
        */
        while (!strstr(cmd, "((") && strstr(cmd, "$")) {
            if (!expand_parameter((char *) cmd, hashtable))
                break;
        }
        /*
        * Expand POSIX arithemtic expansion
        */
        while (strstr(cmd, "$((")) { /* bug: assignment as conditional */
            if ((cmd = expand_calc((char *) cmd)))
                break;
        }

        while (strstr(cmd, "if")) { /* bug: assignment as conditional */
            if ((cmd = if_execute((char *) cmd, hashtable)))
                break;
        }

        /*
          * Now look for the command in the builtin table, and execute
          * the command if found.
          */
        int * reti;
        if (exec_builtin(reti, cmd)) {
            return 0;
        }
        /*
         * The command is not a built-in, so run the program along
         * the PATH list.
         */
        return run_cmd(cmd, background);
    }
    else return 0;
}

int run_cmd(char *cmd, bool background) {

    char buffer[2];
    buffer[0] = '|';
    buffer[1] = '\0';
    struct str_list *chunks = list_split(cmd, buffer);
    struct pipeline *pipe = malloc(chunks->pipes * sizeof *pipe);
    if (pipe == NULL) {
        fprintf(stderr, "malloc failed!\n");
    }
    int i = 0;
    for (i = 0; i < chunks->pipes; i++) {
        pipe[i].data = malloc(sizeof(char **) * BUFFER_SIZE * chunks[i].size);
        if (pipe[i].data == NULL) {
            fprintf(stderr, "malloc failed!\n");
        }
        int j = 0;
        pipe[i].size = chunks[i].size;
        for (j = 0; j < chunks[i].size; j++) {
            if (chunks[i].argv[j] == NULL) {
                chunks[i].argv[j] = '\0';
                break;
            }
            pipe[i].option = malloc(sizeof(int) * 10);
            if (pipe[i].option == NULL) {
                fprintf(stderr, "malloc failed!\n");
            }
            pipe[i].data[j] = strdup(chunks[i].argv[j]);
            if (pipe[i].data[j] == NULL) {
                perror("strdup");
                exit(EXIT_FAILURE);
            }
            * pipe[i].option = * chunks[i].option;
        }
        pipe[i].data[j] = '\0';
    }
    int status = execute_pipeline(chunks->pipes, pipe, background);
    return status;
}


int execute_pipeline(int n, struct pipeline *pipe, bool background) {
    // background = false;
    int status = 0;
    pid_t pid = -2;
    if (n > -1)
        pid = fork();
    if (pid < 0) {
        perror("fork failed");
        return -1;
    }
    /* If we are the child process, then go execute the string.*/
    if (pid == 0) {
        /* spawn(cmd);*/
        fork_pipeline(n, pipe);
    }
    /*
     * We are the parent process.
     * Wait for the child to complete.
     */

    if (!background) {
        while (((pid = waitpid(pid, &status, 0)) < 0) && (errno == EINTR));
        fprintf(stderr, "%d: executed\n", (int) pid);
    }
    if (pid < 0) {
        fprintf(stderr, "Error from waitpid: %s", strerror(errno));
        return -1;
    }
    if (WIFSIGNALED(status)) {
        fprintf(stderr, "pid %ld: killed by signal %d\n",
                (long) pid, WTERMSIG(status));

        return -1;
    }
    return WEXITSTATUS(status);
}

The grammar (with lemon parser) is:

%include
{
#include "types.h"
#include "openshell.h"
#include "assert.h"
}
%syntax_error { fprintf(stderr, "Syntax error\n"); }
%token_type { struct SToken* }
%type expr { int }

%nonassoc EQEQ NOTEQ SET LARGER SMALLER.
%left FOR WHILE OUTR IF.
%left PLUS MINUS.
%left TIMES DIVIDE THEN SEMICOLON.

program ::= expr(A). { setresult(A);  }

expr(A) ::= expr(B) PLUS expr(C). {A = B + C; }
expr(A) ::= expr(B) MINUS expr(C). {A = B - C; }
expr(A) ::= expr(B) TIMES expr(C). {A = B * C; }
expr(A) ::= expr(B) EQEQ expr(C). { if(B==C) {A=1;} else A=0;setresult2(A);}
expr(A) ::= expr(B) NOTEQ expr(C). {if(B==C) {A=0;} else A=1;}
expr(A) ::= expr(B) LARGER expr(C). {if(B>C) {A=1;} else A=0;}
expr(A) ::= expr(B) SMALLER expr(C). {if(B<C) {A=1;} else A=0;}
expr(A) ::= TRUE. { A=1; }
expr(A) ::= FALSE. { A=0; }
expr(A) ::= expr(B) SET expr(C).  {A=C;B=C;}
expr(A) ::= FOR LPAR expr(B) SEMICOLON expr(C) SEMICOLON expr(D) RPAR expr(E). {A = D*B*E*C+1; /* $((for ( 1 == 1 ; 2 == 2 ; 3 == 3 ) 55)) */}
expr(A) ::= WHILE LPAR expr(B) RPAR expr(C).
{
while (B) { printf("%d", C);
}
A=C;printf("\n");
}
expr(A) ::= expr(B) DIVIDE expr(C).
{
    if (C != 0)
    {
        A = B / C;
    }
    else
    {
        fprintf(stderr, "divide by 0");
    }
}
expr(A) ::= LPAR expr(B) RPAR. { A = B; }
expr(A) ::= INTEGER(B).
{
/* printf("the result = %s\n", B->token); */
    A = B->value;
    /*printf("Passed argument: %s\n", B->token);*/
}
expr(A) ::= expr(B) SEMICOLON. { A=B;  }

expr(A) ::= IF expr(B) SEMICOLON THEN expr(C) SEMICOLON FI. {  setresult2(B); A=C; } /* if [ 1 == 1 ];then echo FOO; fi ;fi; if [ true ];then echo FOO; fi if type less > /dev/null;then echo FOO; fi */

expr(A) ::= command . {  A=1; setresult(A);  }

command ::= FILENAME argumentList .{   }
command ::= FILENAME .

argumentList ::= argument .
argument ::= ARGUMENT .
argument ::= FILENAME .{  }
expr(A) ::= expr(B) OUTR FILENAME.{  A=B; }

The entire project is available from my github.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

There is lots of small stuff that muddies seeing the overall flow.

My apologizes for only a primarily low-level review.

  1. Drop though cases. The code appears to be OK. Yet it looks like it might be missing a break; at the end of case '?'. Recommend to add a comment at the end of a case that intentionally lacks a break, return,etc.

        case '?':
            if (optopt == 'c')
                fprintf(stderr, "Option -%c requires an argument.\n", optopt);
            else if (isprint (optopt))
                fprintf(stderr, "Unknown option `-%c'.\n", optopt);
            else
                fprintf(stderr,
                        "Unknown option character `\\x%x'.\n",
                        optopt);
    
            // Add comment
            // Drop though
    
        default: {
            return 1;
        }
    
  2. Remove debug code

            printf("sh OpenShell version 0.1(a)\n");
            printf("Version: %s\n", VERSION);
    
          //  printf ("%s / %s / %s / %s\n",
            //         program_name, version,
              //       build_date, build_git_sha);
    
  3. Obviously missing include files and other code needed before main().

    int main(int argc, char *argv[]) {
        bool donotrun = false;
        struct sigaction new_action, old_action;
        hashtable_t *hashtable = ht_create(65536);
    
  4. Inconsistent indentation. An if() without {} is tolerable, yet not preferred, yet then code breaks a line that would look fine as one. Suggest re-formating.

    // if (old_action.sa_handler != SIG_IGN)
    //     sigaction(SIGINT, &new_action, NULL);
    // ...
    // i = getopt_long(argc, argv, "pc:fvh",
    //                 options, &index);
    
    if (old_action.sa_handler != SIG_IGN) {
        sigaction(SIGINT, &new_action, NULL);
    }
    ...
    i = getopt_long(argc, argv, "pc:fvh", options, &index);
    
  5. Inconsistent formating in `switch statement concerning blank lines. This and other parts hints that OP is not using an automated formatter. Save time. Use an automated formatter. Often these are incorporated within a design environment. Stand alone ones exist. Avoid manual formatting.

            exit(EXIT_SUCCESS);
    
        }
        case 'h': {
            usage();
            exit(EXIT_SUCCESS);
    
        }
        case 'c': {
            cvalue = optarg;
            command(cvalue, hashtable, background);
            exit(EXIT_SUCCESS);
        }
    
        case 'f': {
    
  6. getPath(); is undefined, naked and uncommented. Certainly such a call could return an error, one that is not handled here.

  7. Buffer over flow potential. There is nothing special about 128. Code needs protection/re-work.

            char str[128];
            strcpy(str, line);  // Unsafe
    
  8. For array indexing use size_t.

            // int cc;
            size_t cc;
    
            cc = strlen(buf);
    
  9. Avoid a hacker exploit. buf[0] could be 0 and then buf[cc - 1] is UB. Instead test.

    if (fgets(buf, CMD_LEN - 1, fp) == NULL) {
    ...
    cc = strlen(buf);
    //if (buf[cc - 1] == '\n')
    if (cc > 0 && buf[cc - 1] == '\n')
    
  10. Simplification

    //char buffer[2];
    //buffer[0] = '|';
    //buffer[1] = '\0';
    char buffer[] = "|";
    
  11. Avoid allocation by size of type. Instead by the size of the referenced variable. This post does not show struct pipeline. OTOH, with the right code, we can have less need for it. BTW, pedantic point: good to do allocation math leading with sizeof as that insures math at least at size_t width. Consider malloc(BUFFER_SIZE * chunks[i].size * sizeof *(pipe[i].data)) could overflow the BUFFER_SIZE * chunks[i].size first done in that order.

     // pipe[i].option = malloc(sizeof(int) * 10);
     pipe[i].option = malloc(sizeof *(pipe[i].option) * 10);
    
     // pipe[i].data = malloc(sizeof(char **) * BUFFER_SIZE * chunks[i].size);
     pipe[i].data = malloc(sizeof *(pipe[i].data) * BUFFER_SIZE * chunks[i].size);
    
  12. Allocation test good, but needs an exit. Good to test, but then what? Suggest returning a failure code too. IMO, better to include some diagnostic info like __LINE__, __FUNC__, etc.

    if (pipe == NULL) {
        // fprintf(stderr, "malloc failed!\n");
       fprintf(stderr, "malloc failed! %d\n", __LINE__);
        // add
        return(EXIT_FAILURE);
    }
    
  13. Some systems have a multitude of special integer types. Rather than guess at their width and potentially truncate, just go for the largest supported on printing

     // fprintf(stderr, "pid %ld:\n", (long) pid);
     fprintf(stderr, "pid %lld:\n", (long long) pid);
     // or
     fprintf(stderr, "pid %jd:\n", (intmax_t) pid);
    
  14. Unclear if the result of list_split() is always chunks->pipes > 0. If it could be 0, then malloc(0) returning NULL is not a sign of OOM.

    struct str_list *chunks = list_split(cmd, buffer);
    struct pipeline *pipe = malloc(chunks->pipes * sizeof *pipe);
    // if (pipe == NULL) {
    if (pipe == NULL && chunks->pipes > 0) {
        fprintf(stderr, "malloc failed!\n");
    }
    
  15. Code is certainly wrong - passing an unknown value in reti to exec_builtin(). With that - code is broken - review done.

    int * reti;
    if (exec_builtin(reti, cmd)) {
        return 0;
    }
    
\$\endgroup\$

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.