Method to safely get a string in C

Question

I'm very new to C and want to write a function that safely prompts the user for a string and returns it. While I think I have the safety part handled, I can't return the string to the main method and print it out. Here is my code

//Method to safely read user input
char * getFileName(char* fileName) {
    char* str[50];
    int isValid = 0;
    while (isValid == 0) {
        isValid = 1;
        printf("Enter file name: ");
        fgets(str, 50, stdin);
        if (strlen(str) == 49 && str[48] != '\n') { //http://stackoverflow.com/questions/21691843/how-to-correctly-input-a-string-in-c
            isValid = 0;
            if (strlen(str) > 0 && str[strlen(str) - 1] != '\n') {//http://stackoverflow.com/questions/35136026/simple-loops-and-string-length-in-c?noredirect=1#comment58000759_35136026
                printf("Error! String is too long\n\n");
                do {
                    fgets(str, 50, stdin);
                } while (strlen(str) > 0 && str[strlen(str) - 1] != '\n');
            }
        }
        for (int i = 0; i < strlen(str); i++) {
            if (str[i] == '%') {
                printf("Error: Attempted string format attack\n\n");
                isValid = 0;
            }
        }
    }
    for (int i = 0; i < strlen(str); i++) {
        fileName[i] = str[i];
    }
    return fileName;
}

int main()
{
    //Opening text file
    char fileName[50];
    getFileName(fileName);
    printf(fileName);

    printf("Press Enter to continue...");
    getchar();
    //
    return 0;
}

Running this code prints this result

and I have no idea why. Any help would be greatly appreciated!

`strlen()` iterates over it's argument to compute it's length, store it's value and use it instead of calling it over and over. — Iharob Al Asimi, Feb 06 '16 at 21:33
What are you trying to do? This code seems needlessly complicated. — 5gon12eder, Feb 06 '16 at 21:34
C does not support methods, only functions. And **never** `printf` an externally supplied string directly (i.e. without a format string like `"%s"`). — too honest for this site, Feb 06 '16 at 21:35
@kaylum look at `if (str[i] == '%')` it looks like it's intentionally `printf(fileName)`. — Iharob Al Asimi, Feb 06 '16 at 21:35
@iharob: Agreed in general, But a good compiler will know about pure functions and have the library properly tagged (or use such a library like glibc). — too honest for this site, Feb 06 '16 at 21:38
You have not copied the string terminator with `for (int i = 0; i < strlen(str); i++) { fileName[i] = str[i];`. And you have not stated a clear question. What is the "safe string" requirement? It all seems very muddled. — Weather Vane, Feb 06 '16 at 21:45

Iharob Al Asimi · Answer 1 · 2016-02-06T21:45:49.283

Because you never copy the null terminator. This

for (int i = 0; i < strlen(str); i++)
    fileName[i] = str[i];

only copies all the characters, but not the null terminator. You could use strcpy() since you included string.h as you overuse strlen().

Try like this

int i;
for (i = 0; str[i] != '\0' ; ++i)
    fileName[i] = str[i];
fileName[i] = str[i];

and it should work as you expect it, and really avoid using strlen() like that.

You also have another major problem,

char *str[50];

is an array of 50 char pointers, and that doesn't seem to be what you want but instead

char str[50];

Your code is too complicated and very hard to understand, I think this is what you want

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

char *
getFileName(char *str, size_t size)
{
    int isValid = 0;
    while (isValid == 0)
    {
        size_t length;
        isValid = 1;

        printf("Enter file name: ");
        if (fgets(str, size, stdin) == NULL)
            return NULL;
        length = strlen(str);
        if ((length == size - 1) && (str[size - 2] != '\n'))
        {
            isValid = 0;
            if ((length > 0) && (str[length - 1] != '\n'))
            {
                printf("Error! String is too long\n\n");
                continue;
            }
        }

        if (strchr(str, '%') != NULL)
        {
            printf("Error: Attempted string format attack\n\n");
            isValid = 0;
        }
    }
    return str;
}

int main()
{
    char array[100];
    if (getFileName(array, sizeof(array)) == NULL)
        fprintf(stderr, "error: input problem?\n");
    else
        printf(array);
    return 0;
}

That fixed it, thanks! I'll accept the answer in 7 minutes when I can — Nick Gilbert, Feb 06 '16 at 21:39

score 0 · Answer 2 · answered Feb 06 '16 at 23:17

While you have found the nul-terminator issue that was causing initial problems, there are a couple of other issues you will want to address when taking input. Your choice of fgets is a good one. However, like all line-oriented input functions, it will read up to and including the newline ('\n') into your buffer. This leaves your buffer with an embedded '\n' at the end. You will want to remove or trim the trailing newline as part of your input routine. Otherwise, this will cause problems later on (imagine calling strcat to add to the buffer later).

Handling removal is straight forward. To help yourself in this task, when you call strlen in your function, save the results. This will prevent having to make repeated calls to strlen in your function. With the length known, you can perform your test to check if additional characters remain unread, and following that test, you know whether there is a '\n' located at buffer[len-1] or not. If one is present, just overwrite the '\n' with the nul-terminating character to remove the trailing newline. e.g.

buf[--length] = 0;  /* remove newline, decrement length */

Decrementing the length at the same time, updates the length to the current length reflecting the removal of the newline. Since you have gone to the trouble to compute the length of the string taken as input, it would be nice to make the length available to the calling function without having to repeat the strlen call.

Since you are returning a pointer to the buffer holding the input, one way to do this is to simply pass a pointer for the length as a parameter to your input function, and update the pointer value with the length of the string. That way the length is computed only once and it is available to the calling function immediately upon the function return.

In addition, you can make use to the strchr function to check for your choice of invalid characters in the input. If there are multiple values you would like to consider invalid, or if they change depending on the input your are taking, you can pass a string containing each of the characters as a parameter to your function. Another consideration is presented if an invalid character is found. While your error checking is fine, if you do find an invalid character, you can set your buffer to empty-string by setting the first character in the string to the nul-terminating character.

You can add flexibility to your input function by passing the prompt as a parameter to the function as well. That way, instead of having a function that prompts for and returns a filename, you have a function that can prompt for any input. The general idea is to get away from hard-coding values in your function and, instead, pass values that change as parameters to your function. You can do the same with your character limit as well. So instead of having a function limited to prompting for/reading a filename of 49-chars, you have a general input function that can read any length string into an adequately size character array.

Putting those pieces together, you could greatly increase the flexibility of your input function. The following is a short example function that passes changing information as parameter. Look it over and let me know if you have any questions. Hopefully the example will provide you with a few of the general considerations to consider when writing any function in C. Good luck with your coding:

char *getstr_input (char *buf, size_t max, size_t *n, const char *prompt)
{
    if (!buf) return NULL;
    if (*prompt) printf ("%s : ", prompt);      /* prompt      */
    if (!fgets (buf, max, stdin)) return NULL;  /* read input  */

    if (!(*n = strlen (buf))) return NULL;      /* test strlen */
    if (*n + 1 == max && buf[*n-1] != '\n') {   /* test short-read */
        fprintf (stderr, "warning: short-read, characters remain unread.\n");
    }
    else  /* remove trailing '\n' */
        buf[--*n] = 0;

    if (strchr (buf, '%')) { /* validate unwanted characters  */
        fprintf (stderr, "error: invalid character in string.\n");
        *buf = *n = 0;       /* set to empty-string, 0 length */
        return NULL;
    }
    return buf;
}

Where 'buf' is adequately sized to hold 'max' characters, that on return contains 'n' characters (not including the nul-terminating character) and for which the user is prompted with 'prompt' to input. A short test program could be:

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

enum { MAX = 50 };  /* create a constant 50, adjust as desired */

int main (void) {

    char fname[MAX] = {0};
    size_t len = 0;

    getstr_input (fname, MAX, &len, "enter a filename");

    printf ("\n filename : %s\n length   : %zu\n\n", fname, len);

    return 0;
}

Example Use/Output

$ ./bin/fgets_prompt
enter a filename : 12345678

 filename : 12345678
 length   : 8

$ ./bin/fgets_prompt
enter a filename : 12345%6
error: invalid character in string.

 filename :
 length   : 0

Method to safely get a string in C

2 Answers2