Why is my program printing this C-String?

Question

I'm currently working on a project for my Operating Systems class and have found a weird error where when printing the string produced when processing the text a user enters in the command line. Here is the output of the program:

JR > help
H�E�����UH��SH��(H�}��E�

Here is the cope that produces this output:

/*
* File: hw3.c
* Purpose: To demonstrate a shell by allowing a user to enter commands 
* and perform actions depending on the command.
*/

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

/* Function Prototypes */
void printPrompt();
char* parseCommand(char* cmd);
char* readCommandLine();

/*
 * Entry point of the program.
 */
int main(int argc, char **argv) {
    while(1) {
        printPrompt();

        char* cmdLine = readCommandLine();
        char* cmd = parseCommand(cmdLine);

        printf("Selected Command: %s", cmd);
    }
}

/*
 * This function prints a prompt to the user to enter a command.
 */
void printPrompt() {
    printf("\n%s", "JR > ");
}

/**
 * This function reads the command the user enters from the terminal.
 */
char* readCommandLine() {
    char* input = malloc(10);
    
    fgets(input, 10, stdin);

    return input;
}

char* parseCommand(char* cmd) { 
    char *selectedCommand; /* The command to return */

    /* Convert cmd to all lowercase and store it in enteredCommand. */
    int i;
    for (i = 0; i < strlen(cmd); i++)
        cmd[i] = tolower(cmd[i]);

    if (strcmp(cmd, "cd") == 0)
        selectedCommand = "cd";
    else if(strcmp(cmd, "help") == 0)
        selectedCommand = "help";
    else if(strcmp(cmd, "pwd") == 0)
        selectedCommand = "pwd";
    else if (strcmp(cmd, "exit") == 0)
        selectedCommand = "exit";

    return selectedCommand;
}

Can anyone explain why the printf() function is printing this?

Thank you!

What happens if nothing matches? You return junk. It's possible you're trying to compare `"help\n"` and it fails to match, returning an uninitialized pointer. — tadman, Oct 07 '20 at 00:12
Tip: Try not to use microscopic buffers. Instead use 1024 as a default, and go up from there as necessary. 10 bytes is absolutely ridiculous and sets yourself up for overflow bugs when you use even a moderately long command name or later add arguments, like `"list names"` is too long, that's 10 characters and your buffer can only hold 9 + NUL terminator. — tadman, Oct 07 '20 at 00:13

score 1 · Accepted Answer · answered Oct 07 '20 at 00:26

There's two problems here:

fgets is documented as "if a newline character is found...str will contain that newline character" so you're actually getting "help\n" as an input. You need to strip that newline or anticipate it.
Your if chain fails to address the case when none of those match. You should have a default return NULL handler.

I'd rewrite that chunk to be safer, like this:

char* parseCommand(char* cmd) { 
    // Move declaration inside of for()
    for (int i = 0; i < strlen(cmd); i++)
        cmd[i] = tolower(cmd[i]);

    if (strcmp(cmd, "cd") == 0)
        return "cd";
    else if(strcmp(cmd, "help") == 0)
        return "help";
    else if(strcmp(cmd, "pwd") == 0)
        return "pwd";
    else if (strcmp(cmd, "exit") == 0)
        return "exit";

    return NULL;
}

Where now there's no chance of it falling out the bottom in an inconsistent state.

Technically those could all be simple if statements now, the else is already expressed by virtue of the return.

Why is my program printing this C-String?

1 Answers1