magical segmentation fault?

Question

I've looked at other similar issues on the site but I still don't see what I'm missing. C is a new and terrifying beast for me, so I'm sure it's something simple, but I'm getting segmentation fault(core dump) when the code reaches the fgets(); line just inside the while loop.

I tried writing a string directly to currentInput instead and still get it, so I think I'm somehow accessing the string wrong?

My understanding is that a segmentation fault is caused by accessing memory that (the program?) doesn't have access to...

As an aside, is there a way to use a string literal in strcmp(); so I can just compare to "END"?

void runCommands()
{
    char * currentInput = (char*)malloc(100); //100 char input buffer
    char * cmd = (char*) malloc(CMD_LENGTH);
    char * target = (char*) malloc(UID_LENGTH);
    char * key = (char*) malloc(KEY_LENGTH);
    char * endstr = "END";
    cmd = "UNDEF";
    target = "UNDEF";
    key = "UNDEF";
    ushort tokens;

    while (strcmp(cmd, endstr) != 0) //Run until command is "END"
    {
        printf("ENTER INSTRUCTION: ");
        fgets(currentInput, sizeof(currentInput), stdin); //FAULT OCCURS HERE
        tokens = sscanf(currentInput, "%[^,\n],%[^,\n],%s", cmd, target, key); //parse string for values
        if (tokens <= 3 && tokens >= 1) //ensure valid # of tokens passed
        {
            fprintf(stdout, "TOKENS:\nCMD: %s\ntarget: %s\nkey: %s\n", cmd, target, key);
            switch (tokens)
            //restore UNDEF for non-existent tokens
            {
            case 1:
                target = "UNDEF";
                /* no break */
            case 2: //intentional fallthrough
                key = "UNDEF";
                break;
            default:
                break;
            }
            /* handle commands */
            if (strcmp(cmd, endstr) == 0)
            {
                end(keyfile);
            } //write file and exit function
            else if (strcmp(cmd, "DELETE") == 0)
            {
                delete(target, key);
            } //delete specified key from UID
            else if (strcmp(cmd, "VALIDATE") == 0)
            {
                validate(target, key);
            } //valid/not valid based on key presence
            else if (strcmp(cmd, "ADD") == 0)
            {
                add(target, key);
            } //add key to target UID
            else if (strcmp(cmd, "PRINT") == 0)
            {
                print(target);
            } //print sorted keys for UID or all keys for ALL
            else
            {
                invalidCMD(cmd);
            } //error message for invalid command
        }
        else
        {
            invalidCMD(currentInput); //use whole input as bad command if invalid format
        }
    }
    free(currentInput);
    free(target);
    free(key);
    free(cmd);
}

Answer 1

You are causing a memory leak as well as undefined behavior by overwriting the pointer value returned by malloc:

char * currentInput = (char*)malloc(100); //100 char input buffer
char * cmd = (char*) malloc(CMD_LENGTH);
char * target = (char*) malloc(UID_LENGTH);
char * key = (char*) malloc(KEY_LENGTH);
//...
// This is where you cause the issue
char * endstr = "END";
cmd = "UNDEF";
target = "UNDEF";
key = "UNDEF";

Since you cut the code off, I can't comment on the rest of your code to determine what else would cause an issue.

One thing is that you definitely are not using sizeof() correctly, since sizeof(currentInput) is equal to sizeof(char*), and not the length of the string.

Answer 2

When you write cmd = "UNDEF", you are setting the pointer (that is cmd) to the location of literal array of characters. You should use strcpy if you want to assign strings of characters (and not pointers to them) around.

C compiler will allocate space automatically for string constants, however, it will not allow to rewrite them, which you are probably trying to do in the parts of code you omitted.

Answer 3

You need to read up a bit on C types and pointers.. :) Consider the changes below:

#define CMD_LENGTH 100
#define UID_LENGTH 100
#define KEY_LENGTH 100    
void runCommands()
    {
        char * currentInput = (char*)malloc(100); //100 char input buffer
        char * cmd = (char*) malloc(CMD_LENGTH);
        char * target = (char*) malloc(UID_LENGTH);
        char * key = (char*) malloc(KEY_LENGTH);
        char * endstr = (char *)malloc(sizeof(char) * 100);
        ushort tokens;

        strcpy(cmd, "UNDEF");
        strcpy(target, "UNDEF");
        strcpy(key, "UNDEF");

    while (strcmp(cmd, endstr) != 0) //Run until command is "END"
    {
        printf("ENTER INSTRUCTION: ");
        fgets(currentInput, sizeof(currentInput), stdin);

You cannot use a simple assignment to put a value into a pointer in that way. Doing so effectively overwrites the pointer with whatever "END" (for example) translates to in hex, which is not a memory location that your process owns. In fact, if you receive a segmentation fault it typically means that you tried to read memory that you don't own while an access violation is typically an attempt to write to memory that you don't own.

I expect that much of your trouble is being caused by overwriting pointers inadvertently.

It is also very bad practice to use "magic numbers"... (magically allocating 100 bytes, for example. How do you know it's 100? Will it always be 100?) You should also verify that malloc actually returned a value rather than assuming that it did. The more typical way of calling Malloc has also been illustrated, though I did not validate the return. Finally, fgets is super dangerous since it is an unbounded read. You'd prefer to use functions that allow you to specify the maximum size to read to avoid overflow conditions on the heap or the stack (depending if it's a local variable, global variable or pointer to the heap).