How to Get User Input from console -- safely.

Good notion, but it doesn't work. I input "Now is the time for all good men to come to the aid of their country." and all I got back was "untry". I was expecting to get back the first 20 characters of what I had typed, and your function to toss the rest of the characters into the bit bin.

int main()
{
    char buf[20] = {0};
    int x = getUserInput (buf, sizeof(buf));
    printf("x = %d\n%s\n", x,buf);
}

Now is the time for all good men to come to the aid of their country.
x = 69
untry.
Press any key to continue . . .

Oh, damn. you're right, it is broken.

my original was working by having the first fgets outside of the while loop with a second fgets inside. at the last minute, I condensed it all into a single do/while loop to make it more concise ... and neglected to perform a basic challenge to check my code

:icon_redface:

moving the strcpy to inside the do/while loop as a conditional fixes it, so it performs correctly.

thanks for checking it out and letting me know. how embarrassing.

here is the fixed version:

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

//*****************************************************************************************//
// GetUserInput()
//
// (c) 2010 by Jephthah and distributed for general use under the WTFPL licence
//
// purpose:gets user input, removes the newline, passes back results up to maximum
//         number of characters, flushes remaining characters from input buffer,
//         returns the number of total characters entered by user
//
// input:  maxStringLength is the maximum allowble characters to input by user
//
// output: returnStr, contains up to the maximum length of characters allowed that were
//         input by the user, any additional characters entered are lost
//
// return: total characters entered by user; caller should check that this value is equal
//         to or less than the maximum allowed to indicate valid string was input.  larger
//         value returned than was allowed by the input indicates characters were lost
//
//*****************************************************************************************//
int getUserInput (char * returnStr, int maxStringLength)
{
   char    *tempStr;
   int     totalCount = 0, overflow = 0;
   size_t  len;

   tempStr = malloc((maxStringLength+2) * sizeof(char));  //temporary holder

   do {
      fgets(tempStr, maxStringLength+1, stdin);  // get chars from input buffer
      len = strlen(tempStr);

      if (tempStr[len-1] == '\n') // newline indicates end of string
      {
         tempStr[len-1] = '\0';   // delete it
         len = strlen(tempStr);   // and recalc length
      }

      if (!overflow++)
         strcpy(returnStr,tempStr);  // copy first string into output

      totalCount += (int)len;
   }
   while ((int)len == maxStringLength);  // continue to flush extras if too long

   free(tempStr);              // and release memory

   return totalCount;   // may be more than the number allowed
}

int main()
{
   char buf[20] = {0};
   int x;
   while(1)
   {
      printf("Enter String: ");
      fflush(stdout);
      x = getUserInput (buf, sizeof(buf));
      printf("Entered %d Chars Total.\nSaved Result: %s\n\n", x, buf);
   }
   return 0;

}

@Jephthah: (In your fixed snippet)
You can replace line 38 ( len = strlen(tempStr); ) by len--; :)

true. it must've made sense at one time why i did it the long way but, a few edits later, i can't recall what it was.

:)

strcpy() and strlen() requires the header file string.h included.
malloc()'s return needs to be checked before using the memory.

jephthah>

// return: total characters entered by user; caller should check that this value is equal
// to or less than the maximum allowed to indicate valid string was input. larger
// value returned than was allowed by the input indicates characters were lost

I find the return of the string or null more useful that the total amount of characters.

char *getUserInput (char * returnStr, int maxStringLength)

With such I could even:

printf("The text entered was %s\n", getUserInput( buf, sizeof(buf)));

Now, instead of using fgets(), malloc(), strcpy(), strlen(), the same result could be achieved with just getchar() and a couple ifs:

#include <stdio.h>

size_t obtainput (char *orig, size_t size)
{
    size_t i = 0;
    size_t  total = 0;

    --size;
    for (;;) {
        int chr = getchar();
        if (chr == '\n' || chr == EOF) break;
        if (i < size) orig[i++] = chr;

        ++total;
    }
    orig[i] = '\0';
    return total;
}

In line 29: [B]sizeof(char)[/B] is - by definition - equal to 1.
So this: tempStr = malloc((maxStringLength+2) * [B]sizeof(char)[/B]); .
Could be rewritten as: tempStr = malloc(maxStringLength+2) .
However leaving the sizeof out won't increase performance, since it is evaluated at compile-time, it's just a matter of personal preference.

whats wrong with scanf?

whats wrong with scanf?

http://www.gidnetwork.com/b-59.html
http://www.gidnetwork.com/b-60.html
http://www.gidnetwork.com/b-62.html
http://www.gidnetwork.com/b-63.html
http://www.gidnetwork.com/b-64.html

Since this thread's title is "How to Get User Input from console -- safely.", I think it's appropriate to point out a little thing. Namely, in the Aia's version, decrementing size before entering the for-loop is a recipe for buffer overflow if size 's initial value (for whatever reason) is zero.
So, to be on safer side ..

size_t obtainput (char *orig, size_t size)
{
    size_t i = 0;
    size_t  total = 0;
	
    /* Safety check ... */
    if(size == 0)
        return size;

    --size;
...

Both versions of getUserInput will put the null terminator just past the end of returnStr, in returnStr[maxStringLength], if at least maxStringLength-1 characters are read (and there is, as a result, no \n to replace with \0). Try watching, with a debugger, what happens to returnStr when you type at least maxStringLength-1 characters.

This is assuming that maxStringLength == sizeof(returnStr), but that's the way you used it, and the way I would expect it to work.

I think you can fix it just by always setting tempStr[maxStringLength-1] to \0 before you copy it into returnStr. Even if there's already a \0 closer to the beginning of tempStr, it wouldn't cause any problems.