Trouble creating a shell in C (Seg-Fault and ferror)

Question

I have been following a tutorial on how to make my own shell but I have been stuck for a couple days now.

Two things:

1. When this code is compiled and ran, it will randomly have segmentation faults and I cannot figure out why.
2. The if statement `if (ferror != 0)` always seems to be true. which is odd because I do not understand why fgets() is failing in the main() function.

Any information on these topics (or other topics about creating this shell) would be greatly appreciated.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/types.h>

#define MAXSIZE 512

int parseCmd(char *cmd, char *args[])
{
    printf("LOGGER: parseCmd(cmd=%s, args=%p)\n", cmd, args);

    char cmdDelims[] = {' ','>'};

    char *cmdReader;
    cmdReader = strtok(cmd, cmdDelims);

    int i = 0;
    while (cmdReader != NULL)
    {       
        args[i] = strdup(cmdReader);

        printf("LOGGER: args[%d]=%s\n", i, args[i]);

        cmdReader = strtok(NULL, cmdDelims);
        i++;
    }
    return 0;
}

void printToLine(char *args[])
{
    int length;
    length = sizeof(args) / sizeof(char);

    int i = 0;
    while (i < length)
    {
        printf("%s\n", args[i]);
        i++;
    }
}

int main(int argc, char *argv[]) 
{   
    char *in;
    in = malloc(MAXSIZE);

    char *args[15];
    char *cmd = NULL;

    int errorBit = 0;
    int terminationBit = 1;
    char error_message[30] = "An error has occurred\n";

    char inDelims[] = "\n";

    while (terminationBit)
    {
        printf("mysh>");

        // get input from command line
        fgets(in, MAXSIZE, stdin);
        if (ferror != 0)
        {
            perror(error_message);
        }

        // get pointer to command line input w/o the newline
        cmd = strtok(in, inDelims);

        // parse the command into separate arguments
        errorBit = parseCmd(cmd, args);
        if (errorBit)
        {
            perror(error_message);
            exit(1);
        }

        printToLine(args);

        // check if the user wants to exit the shell
        if (strcmp(*args, "exit") == 0)
        {
            terminationBit = 0;
        }
    }
    return 0;
}

Here are some outputs:

**[ray@12] (6)$ mysh**
mysh>1 2 3
An error has occurred
: Success
LOGGER: parseCmd(cmd=1 2 3, args=0x7fff4a50b080)
LOGGER: args[0]=1
LOGGER: args[1]=2
LOGGER: args[2]=3
1
2
3
Segmentation fault (core dumped)
**[ray@12] (7)$ mysh**
mysh>1 2 3 4 5 6 7 8 9 10
An error has occurred
: Success
LOGGER: parseCmd(cmd=1 2 3 4 5 6 7 8 9 10, args=0x7fffba053d70)
LOGGER: args[0]=1
LOGGER: args[1]=2
LOGGER: args[2]=3
LOGGER: args[3]=4
LOGGER: args[4]=5
LOGGER: args[5]=6
LOGGER: args[6]=7
LOGGER: args[7]=8
LOGGER: args[8]=9
LOGGER: args[9]=10
1
2
3
4
5
6
7
8
mysh>1 2 3
An error has occurred
: Success
LOGGER: parseCmd(cmd=1 2 3, args=0x7fffba053d70)
LOGGER: args[0]=1
LOGGER: args[1]=2
LOGGER: args[2]=3
1
2
3
4
5
6
7
8

Schade

Why don't you check the return value from fgets? You can't detect EOF by checking ferror(stdin). – Brackely

After looking more closely I am assuming I need to free my args variable before I reuse it. I do not understand why I get seg-faults for the parsing of an array with only 3 elements though. – Schade

See my update answer for that one – Runkle

Answer 1

For the ferror error, you need to test ferror(stdin), not ferror. The latter is the function address, which will never be zero:

if (ferror(stdin) != 0)
{
    perror(error_message);
}

For at least some of the segfaults, this does not do what you think:

length = sizeof(args) / sizeof(char);

This will tell you how many bytes are used to store the pointer, which is 4 or 8 depending, and not the number of arguments.

So if you have four (or eight) arguments, it will appear to work. If you have more, it will seem to ignore some arguments. And if you have less, it will fetch the missing arguments from across the Void, resulting in a (almost sure) segmentation fault.

You need to calculate length independently and pass it along, or store some terminator in args, for example adding a NULL argument after the last valid argument you find:

        cmdReader = strtok(NULL, cmdDelims);
        i++;
    }
    args[i] = NULL;
    return 0;
}

void printToLine(char *args[])
{
    int i = 0;
    while (args[i])
    {
        printf("%s\n", args[i]);
        i++;
    }
}

Runkle