Segmentation Fault With Strcmp With One Input

Question

I am having an issue with the following code.

I have a global variable

char tokens[512][80];

Along with code:

int main(int argc, char** argv) {
    char *input = malloc(sizeof(char) * 80);

    while (1) {
        printf("mini-shell>");
        fgets(input, 80, stdin);
        parse(input);

        if (strcmp(tokens[0], "cd") == 0) {
            cd();
        }
        else if (strcmp(tokens[0], "exit") == 0) {
            exit(1);
        }
    }
}

void parse(char str[]) {
    int index = 0;
    char* str_ptr = strtok(str, " ");

    while (str_ptr != NULL) {
        strcpy(tokens[index], str_ptr);
        str_ptr = strtok(NULL, " \0\r\n");
        //printf("%d\n", index);
        index = index + 1;
    }
}

I found that if I enter exit for stdin I get a Segmentation fault, but if I enter cd .. for stdin I don't. Why is this so?

Answer 1

We don't know what the definition of the cd() function is, but there are a number of things that you may wish to consider in this program.

First, I don't believe there's any benefit to dynamically allocating 80 bytes of memory for the input buffer when you can easily do so automatically on the stack with char input[80]; - this is free and easy and requires no deallocation when you're done.

If you do this, you derive the size with fgets(input, sizeof input, stdin) where if you change the size of your input line from 80 to some other number, you only have to change it once: the sizeof on an array pulls the size directly.

Your parse() routine needs a little bit of help also. It's a really good idea to declare the function via the extern as shown so that when the compiler sees you call the function in the loop (right after the fgets), it knows the parameter and return types. Otherwise it has to make assumptions.

Because parse() is splitting apart the line you read from input, it's not required to copy the strings to some other place, so you can turn tokens from a multi-dimensional array into a simple array of pointers. As you run strtok() through the line to split up the parameters, you can store just the pointer, knowing that they will be pointing to stable data until the next fgets().

Also: your code does not strictly require or use this, but adding a NULL pointer to the end of the tokens list is a really good idea: otherwise, how does the caller know how many parameters were actually entered? This code checks whether the user entered just a blank line or not.

We've also change the loop around a little bit so the strtok() is called just once instead of twice, including the \n as noted in the comments.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

char *tokens[512];

extern void parse(char *str);

int main(int argc, char** argv) {
    char input[80];

    while (1) {
        printf("mini-shell> "); fflush(stdout); // make sure user sees prompt
        fgets(input, sizeof input, stdin);
        parse(input);

        if (tokens[0] == NULL) continue; // user entered blank line

        if (strcmp(tokens[0], "cd") == 0) {
            cd();
        }
        else if (strcmp(tokens[0], "exit") == 0) {
            exit(1);
        }
    }
}

void parse(char *str) {
    int index = 0;

    char* str_ptr;

    while ( (str_ptr = strtok(str, " \n")) != NULL)
    {
        tokens[index++] = str_ptr;
        str = NULL; // for next strtok() loop
    }
    tokens[index] = NULL;
}