More error-checking

Question

The purpose of this code is to check for errors in my code for a custom shell.

In a previous answer, they say that my code didn't have error-checking.

You need to check the result of every function that might fail

So I've been asking about how to do it.

http://stackoverflow.com/questions/37566840/assert-and-error-checking-in-c

Now I could put back the assertions and they worked. I also do run-time checking of strdup like it was mentioned in the CR answer:

int run_cmd(const 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);
    int i = 0;
    for (i = 0; i < chunks->pipes; i++) {
        pipe[i].data = malloc(sizeof(char **) * BUFFER_SIZE * chunks[i].size);
        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); 
            chunks[i].option = malloc(sizeof(int) * 10); 
            chunks[i].option[i] = 0;
            pipe[i].data[j] = strdup(chunks[i].argv[j]);
            if (pipe[i].data[j] == NULL) {
                perror("strdup");
                exit(EXIT_FAILURE);
            }
            pipe[i].option[0] = chunks[i].option[i];
        }
        pipe[i].data[j] = '\0';
    }
    int status = execute_pipeline(chunks->pipes, pipe, background);
    return status;
}




char *if_execute(char *shellcommand) {

    char mystring[CMD_LEN];
    void *pParser;
    char *c;

    shellcommand = str_replace(shellcommand, ";", " ; ");
    reti = regcomp(&regex, "[0-9]==[0-9]", 0);
    if (reti) {
        fprintf(stderr, "Could not compile regex\n");
        exit(1);
    }

    /* Execute regular expression */
    reti = regexec(&regex, shellcommand, 0, NULL, 0);
    if (!reti) {
        shellcommand = str_replace(shellcommand, "==", " == ");;
    }
    else if (reti == REG_NOMATCH) {
        /* puts("No match"); */
    }
    else {
        regerror(reti, &regex, msgbuf, sizeof(msgbuf));
        fprintf(stderr, "Regex match failed: %s\n", msgbuf);
        exit(1);
    }

    char *line = strcpy(mystring, shellcommand);
    pParser = (void *) ParseAlloc(malloc);
    if (line) {
        char *buf[64];
        struct SToken v[32];
        int value;
        char **ptr1 = str_split(buf, line, ' ');
        int j = 0;
        for (j = 0; ptr1[j]; j++) {
            c = ptr1[j];
            char *c2 = strdup(c);
            if (c2 == NULL) {
                perror("strdup");
                exit(EXIT_FAILURE);
            }
            v[j].token = c2;
            switch (*c2) {
                case '0':
                case '1':
                case '2':
                case '3':
                case '4':
                case '5':
                case '6':
                case '7':
                case '8':
                case '9':
                    for (value = 0; *c2 && *c2 >= '0' && *c2 <= '9'; c2++)
                        value = value * 10 + (*c2 - '0');
                    v[j].value = value;
                    Parse(pParser, INTEGER, &v[j]);
                    continue;
            }

            if (!strcmp("if", c)) {
                Parse(pParser, IF, NULL);
            }
            else if (!strcmp("true", c)) {
                Parse(pParser, TRUE, NULL);
            }
            else if (!strcmp("then", c)) {
                Parse(pParser, THEN, NULL);
                char *token = "then ";
                const char *p1 = strstr(shellcommand, token) + strlen(token);
                const char *p2 = strstr(p1, ";");
                size_t len = p2 - p1;
                char *res = (char *) malloc(sizeof(char) * (len + 1));
                strncpy(res, p1, len);
                res[len] = '\0';

                if (result2)
                    shellcommand = res;
                else
                    shellcommand = "echo";
            }
            else if (!strcmp("[", c)) {
                Parse(pParser, LSBR, NULL);
            }
            else if (!strcmp("]", c)) {
                Parse(pParser, RSBR, NULL);
            }
            else if (!strcmp(";", c)) {
                Parse(pParser, SEMICOLON, NULL);
            }
            else if (!strcmp("fi", c)) {
                Parse(pParser, FI, NULL);
            }
            else if (strlen(c) > 0 && strstr(c, "==")) {
                v[j].token = c;
                Parse(pParser, EQEQ, &v[j]);
            }
            else {
                Parse(pParser, FILENAME, NULL);
            }
        }
        Parse(pParser, 0, NULL);
    }

    return shellcommand;
}









int do_checkenv(int argc, const char **argv) {
    int status;
    int len = 1;
    char *grep[4];
    char *tmp = NULL;
    int k;
    char *pagerValue;
    int pos = 0;
    int i = 0;
    struct command shellcommand[4];
    char *pager_cmd[] = {"less", 0};
    char *printenv[] = {"printenv", 0};
    char *sort[] = {"sort", 0};
    char *path_strdup;
    char *path_value;
    char *pathValue;
    pid_t pid;
    pathValue = getenv("PATH");
    path_strdup = strdup(pathValue);
    if (path_strdup == NULL) {
        perror("strdup");
        exit(EXIT_FAILURE);
    }
    path_value = strtok(path_strdup, ":");
    if (find_less_program(path_value)) {
        pager_cmd[0] = "less";
    }
    pagerValue = getenv("PAGER");
    if (!pagerValue) {
        if (find_less_program(path_value)) {
            pager_cmd[0] = "less";
        } else {
            pager_cmd[0] = "more";
        }
    }
    else {
        pager_cmd[0] = pagerValue;
    }

    if (i == 1) {
        /* do nothing */
    }
    else {
        for (k = 1; k < i; k++) {
            len += strlen(argv[k]) + 2;
        }
        tmp = (char *) malloc(len);
        tmp[0] = '\0';
        for (k = 1; k < argc; k++) {
            pos += sprintf(tmp + pos, "%s%s", (k == 1 ? "" : "|"), argv[k]);
        }
        grep[0] = "grep";
        grep[1] = "-E";
        grep[2] = tmp;
        grep[3] = NULL;
        shellcommand[0].argv = printenv;
        shellcommand[1].argv = grep;
        shellcommand[2].argv = sort;
        shellcommand[3].argv = pager_cmd;
        fflush(NULL);

        pid = fork();

        if (pid < 0) {
            perror("fork failed");
            free(path_strdup);
            free(tmp);
            return -1;
        }

        if (pid == 0) {
            fork_pipes(4, shellcommand);

        }
        /*
            * We are the parent process.
            * Wait for the child to complete.
            */
        status = 0;

        while (((pid = waitpid(pid, &status, 0)) < 0) && (errno == EINTR));

        if (pid < 0) {
            fprintf(stderr, "Error from waitpid: %s", strerror(errno));
            free(path_strdup);
            free(tmp);
            return -1;
        }

        if (WIFSIGNALED(status)) {
            fprintf(stderr, "pid %ld: killed by signal %d\n",
                    (long) pid, WTERMSIG(status));
            free(path_strdup);
            free(tmp);
            return -1;
        }
        free(path_strdup);
        free(tmp);
        return WEXITSTATUS(status);

    }
    free(path_strdup);
    free(tmp);
    return 1;
}



/* Returns a struct that has the number of "chunks" the list of chunks.
 * Splits the command by char | and then by whitespace and return a list of struct pointers
 */
struct str_list *list_split(const char *a_str, char *a_delim) {
    assert(a_str);
    assert(a_delim);
    char **result = 0;
    size_t count = 0;
    char *tmp = (char *) a_str;
    char *ctmp = "";
    char *token = "";
    char *last_comma = 0;
    char *tmp2 = (char *) a_str; /* TODO: This variable can reuse tmp */
    struct str_list *chunks = NULL;
    /* Count how many elements will be extracted. */
    while (*tmp) {
        if (*a_delim == *tmp) {
            count++;
            last_comma = tmp;
        }
        tmp++;
    }
    /* Add space for trailing token. */
    count += last_comma < (a_str + strlen(a_str) - 1);
    count++;
    result = alloc_argv(count);
    char **tmpresult = alloc_argv(count);
    chunks = malloc(count * sizeof *chunks);
    if (result == NULL) {
        printf("Error allocating memory!\n");
        return chunks;;
    }
    if (result) {
        size_t idx = 0;
        token = strtok((char *) strdup(a_str), "|");
        int a = 0;
        while (token) {
            assert(idx < count);
            tmpresult[a] = strdup(token);
            a++;
            ctmp = strdup(token);
            if (ctmp == NULL) {
                perror("strdup");
                exit(EXIT_FAILURE);
            }
            *(result + idx++) = ctmp; /* memory leak! how to free() */;
            token = strtok(0, a_delim);
        }
        assert(idx == count - 1);
        *(result + idx) = 0;
    }
    chunks->argv = alloc_argv(BUFFER_SIZE);
    int i = 0;
    chunks = tokenize(&i, chunks, result, count, tmp2);
    chunks->pipes = i; /* important! to get this right */
    free(ctmp);
    return expand_shell(tmpresult, chunks);
}













/* Returns a struct that has the number of "chunks" the list of chunks.
 * Splits the command by char | and then by whitespace and return a list of struct pointers
 */
struct str_list *list_split(const char *a_str, char *a_delim) {
    assert(a_str);
    assert(a_delim);
    char **result = 0;
    size_t count = 0;
    char *tmp = (char *) a_str;
    char *ctmp = "";
    char *token = "";
    char *last_comma = 0;
    char *tmp2 = (char *) a_str; /* TODO: This variable can reuse tmp */
    struct str_list *chunks = NULL;
    /* Count how many elements will be extracted. */
    while (*tmp) {
        if (*a_delim == *tmp) {
            count++;
            last_comma = tmp;
        }
        tmp++;
    }
    /* Add space for trailing token. */
    count += last_comma < (a_str + strlen(a_str) - 1);
    count++;
    result = alloc_argv(count);
    char **tmpresult = alloc_argv(count);
    chunks = malloc(count * sizeof *chunks);
    if (result == NULL) {
        printf("Error allocating memory!\n");
        return chunks;;
    }
    if (result) {
        size_t idx = 0;
        token = strtok((char *) strdup(a_str), "|");
        int a = 0;
        while (token) {
            assert(idx < count);
            tmpresult[a] = strdup(token);
            if (tmpresult[a]  == NULL) {
                perror("strdup");
                exit(EXIT_FAILURE);
            }
            a++;
            ctmp = strdup(token);
            if (ctmp == NULL) {
                perror("strdup");
                exit(EXIT_FAILURE);
            }
            *(result + idx++) = ctmp; /* memory leak! how to free() */;
            token = strtok(0, a_delim);
        }
        assert(idx == count - 1);
        *(result + idx) = 0;
    }
    chunks->argv = alloc_argv(BUFFER_SIZE);
    int i = 0;
    chunks = tokenize(&i, chunks, result, count, tmp2);
    chunks->pipes = i; /* important! to get this right */
    free(ctmp);
    return expand_shell(tmpresult, chunks);
}

Now I added error-checking to all calls to strdup, do you see more error-checking that I can do, based on the code I posted? The whole project is available in my github.

Where to deallocate (free()) the memory? All the mallocs and strdup() in run_cmd() allocate memory that needs to be deallocated or you have a memory leak problem. — pacmaninbw, 2 days ago
Function list_split() appears twice with only slight variations. Suggest review. — chux, 2 days ago

chux · Answer 1 · 2016-06-05 00:50:30Z

do you see more error-checking that I can do, based on the code I posted?

Check for allocation failure. (many places)

pipe[i].option = malloc(sizeof(int) * 10); 
if (pipe[i].option == NULL) Handle_OOM();

Unclear based on posted code if shellcommand could get longer. If so, recommend passing in the buffer size to str_replace().
```
shellcommand = str_replace(shellcommand, ";", " ; ");
```
Magic numbers. No idea if 64/32 is sufficient. Consider a constant value derived from other limits if variables, use dynamic sized array or something, but not an unadorned constant.
```
char *buf[64];
struct SToken v[32];
```

Code appears to depend on p1 containing a ';'. I do not see that and therefore suggest a NULL test before p2 - p1

       const char *p2 = strstr(p1, ";");
       if (p2 == NULL) Handle_It(); 
       size_t len = p2 - p1;

int overflow. Do not see a prevention against value overflow whihc is UB.

if (value >= INT_MAX/10 && (value > INT_MAX/10 || (*c2 - '0') > INT_MAX%10))
    Handle_Overflow();
value = value * 10 + (*c2 - '0');

Pedantic: Use size_t for len.

int do_checkenv(int argc, const char **argv) {
  // int len = 1;
  size_t len = 1;

Do not defeat const

// char *tmp = (char *) a_str;
const char *tmp = a_str;
// char *tmp2 = (char *) a_str; 
const char *tmp2 = a_str;

Avoid subtraction with unsigned types. Maybe a_str will be ""?

// count += last_comma < (a_str + strlen(a_str) - 1);
count += (last_comma + 1) < (a_str + strlen(a_str));

Minor

Allocate to the size of the de-referenced pointer, not type. Is int the correct type to use here? Maybe -maybe not. A review needs to go someplace to the definition of field option to find out. OTOH, the next line of code is certainly allocating the correct size.
```
//  pipe[i].option = malloc(sizeof(int) * 10); 
pipe[i].option = malloc(sizeof *(pipe[i].option) * 10); 
```
Why cast allocations sometimes and not others? Suggest dropping the casts.

Is if_execute() using a global reti? Tsk - tsk!

char *if_execute(char *shellcommand) {
  ...
  reti = regcomp(&regex, "[0-9]==[0-9]", 0);

Maybe more, but GTG.

asked	2 days ago
viewed	28 times
active	2 days ago

current community

your communities

more stack exchange communities

More error-checking

1 Answer 1

Your Answer

Not the answer you're looking for? Browse other questions tagged c error-handling shell or ask your own question.

Linked

Hot Network Questions

current community

your communities

more stack exchange communities

More error-checking

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c error-handling shell or ask your own question.

Linked

Related

Hot Network Questions