C - scanf() vs gets() vs fgets()

Question

I've been doing a fairly easy program of converting a string of Characters (assuming numbers are entered) to an Integer.

After I was done, I noticed some very peculiar "bugs" that I can't answer, mostly because of my limited knowledge of how the scanf(), gets() and fgets() functions work. (I did read a lot of literature though.)

So without writing too much text, here's the code of the program:

#include <stdio.h>

#define MAX 100

int CharToInt(const char *);

int main()
{
    char str[MAX];

    printf(" Enter some numbers (no spaces): ");
    gets(str);
//  fgets(str, sizeof(str), stdin);
//  scanf("%s", str);

    printf(" Entered number is: %d\n", CharToInt(str));

    return 0;
}

int CharToInt(const char *s)
{
    int i, result, temp;

    result = 0;
    i = 0;

    while(*(s+i) != '\0')
    {
        temp = *(s+i) & 15;
        result = (temp + result) * 10;
        i++;
    }

    return result / 10;
}

So here's the problem I've been having. First, when using gets() function, the program works perfectly.

Second, when using fgets(), the result is slightly wrong because apparently fgets() function reads newline (ASCII value 10) character last which screws up the result.

Third, when using scanf() function, the result is completely wrong because first character apparently has a -52 ASCII value. For this, I have no explanation.

Now I know that gets() is discouraged to use, so I would like to know if I can use fgets() here so it doesn't read (or ignores) newline character. Also, what's the deal with the scanf() function in this program?

Answer 1

• Never use gets. It offers no protections against a buffer overflow vulnerability (that is, you cannot tell it how big the buffer you pass to it is, so it cannot prevent a user from entering a line larger than the buffer and clobbering memory).

• Avoid using scanf. If not used carefully, it can have the same buffer overflow problems as gets. Even ignoring that, it has other problems that make it hard to use correctly.

• Generally you should use fgets instead, although it's sometimes inconvenient (you have to strip the newline, you must determine a buffer size ahead of time, and then you must figure out what to do with lines that are too long–do you keep the part you read and discard the excess, discard the whole thing, dynamically grow the buffer and try again, etc.). There are some non-standard functions available that do this dynamic allocation for you (e.g. getline on POSIX systems, Chuck Falconer's public domain ggets function). Note that ggets has gets-like semantics in that it strips a trailing newline for you.

Answer 2

Yes, you want to avoid gets. fgets will always read the new-line if the buffer was big enough to hold it (which lets you know when the buffer was too small and there's more of the line waiting to be read). If you want something like fgets that won't read the new-line (losing that indication of a too-small buffer) you can use fscanf with a scan-set conversion like: "%N[^\n]", where the 'N' is replaced by the buffer size - 1.

One easy (if strange) way to remove the trailing new-line from a buffer after reading with fgets is: strtok(buffer, "\n"); This isn't how strtok is intended to be used, but I've used it this way more often than in the intended fashion (which I generally avoid).

Answer 3

There are numerous problems with this code. We'll fix the badly named variables and functions and investigate the problems:

• First, CharToInt() should be renamed to the proper StringToInt() since it operates on an string not a single character.

• The function CharToInt() [sic.] is unsafe. It doesn't check if the user accidentally passes in a NULL pointer.

• It doesn't validate input, or more correctly, skip invalid input. If the user enters in a non-digit the result will contain a bogus value. i.e. If you enter in N the code *(s+i) & 15 will produce 14 !?

• Next, the nondescript temp in CharToInt() [sic.] should be called digit since that is what it really is.

• Also, the kludge return result / 10; is just that -- a bad hack to work around a buggy implementation.

• Likewise MAX is badly named since it may appear to conflict with the standard usage. i.e. #define MAX(X,y) ((x)>(y))?(x):(y)

• The verbose *(s+i) is not as readable as simply *s. There is no need to use and clutter up the code with yet another temporary index i.

gets()

This is bad because it can overflow the input string buffer. For example, if the buffer size is 2, and you enter in 16 characters, you will overflow str.

scanf()

This is equally bad because it can overflow the input string buffer.

You mention "when using scanf() function, the result is completely wrong because first character apparently has a -52 ASCII value."

That is due to an incorrect usage of scanf(). I was not able to duplicate this bug.

fgets()

This is safe because you can guarantee you never overflow the input string buffer by passing in the buffer size (which includes room for the NULL.)

getline()

A few people have suggested the C POSIX standard getline() as a replacement. Unfortunately this is not a practical portable solution as Microsoft does not implement a C version; only the standard C++ string template function as this SO #27755191 question answers. Microsoft's C++ getline() was available at least far back as Visual Studio 6 but since the OP is strictly asking about C and not C++ this isn't an option.

Misc.

Lastly, this implementation is buggy in that it doesn't detect integer overflow. If the user enters too large a number the number may become negative! i.e. 9876543210 will become -18815698?! Let's fix that too.

This is trivial to fix for an unsigned int. If the previous partial number is less then the current partial number then we have overflowed and we return the previous partial number.

For a signed int this is a little more work. In assembly we could inspect the carry-flag, but in C there is no standard built-in way to detect overflow with signed int math. Fortunately, since we are multiplying by a constant, * 10, we can easily detect this if we use an equivalent equation:

n = x*10 = x*8 + x*2

If x*8 overflows then logically x*10 will as well. For a 32-bit int overflow will happen when x*8 = 0x100000000 thus all we need to do is detect when x >= 0x20000000. Since we don't want to assume how many bits an int has we only need to test if the top 3 msb's (Most Significant Bits) are set.

Additionally, a second overflow test is needed. If the msb is set (sign bit) after the digit concatenation then we also know the number overflowed.

Code

Here is a fixed safe version along with code that you can play with to detect overflow in the unsafe versions. I've also included both a signed and unsigned versions via #define SIGNED 1

#include <stdio.h>
#include <ctype.h> // isdigit()

// 1 fgets
// 2 gets
// 3 scanf
#define INPUT 1

#define SIGNED 1

// re-implementation of atoi()
// Test Case: 2147483647 -- valid    32-bit
// Test Case: 2147483648 -- overflow 32-bit
int StringToInt( const char * s )
{
    int result = 0, prev, msb = (sizeof(int)*8)-1, overflow;

    if( !s )
        return result;

    while( *s )
    {
        if( isdigit( *s ) ) // Alt.: if ((*s >= '0') && (*s <= '9'))
        {
            prev     = result;
            overflow = result >> (msb-2); // test if top 3 MSBs will overflow on x*8
            result  *= 10;
            result  += *s++ & 0xF;// OPTIMIZATION: *s - '0'

            if( (result < prev) || overflow ) // check if would overflow
                return prev;
        }
        else
            break; // you decide SKIP or BREAK on invalid digits
    }

    return result;
}

// Test case: 4294967295 -- valid    32-bit
// Test case: 4294967296 -- overflow 32-bit
unsigned int StringToUnsignedInt( const char * s )
{
    unsigned int result = 0, prev;

    if( !s )
        return result;

    while( *s )
    {
        if( isdigit( *s ) ) // Alt.: if (*s >= '0' && *s <= '9')
        {
            prev    = result;
            result *= 10;
            result += *s++ & 0xF; // OPTIMIZATION: += (*s - '0')

            if( result < prev ) // check if would overflow
                return prev;
        }
        else
            break; // you decide SKIP or BREAK on invalid digits
    }

    return result;
}

int main()
{
    int  detect_buffer_overrun = 0;

    #define   BUFFER_SIZE 2    // set to small size to easily test overflow
    char str[ BUFFER_SIZE+1 ]; // C idiom is to reserve space for the NULL terminator

    printf(" Enter some numbers (no spaces): ");

#if   INPUT == 1
    fgets(str, sizeof(str), stdin);
#elif INPUT == 2
    gets(str); // can overflows
#elif INPUT == 3
    scanf("%s", str); // can also overflow
#endif

#if SIGNED
    printf(" Entered number is: %d\n", StringToInt(str));
#else
    printf(" Entered number is: %u\n", StringToUnsignedInt(str) );
#endif
    if( detect_buffer_overrun )
        printf( "Input buffer overflow!\n" );

    return 0;
}

Answer 4

You're correct that you should never use gets. If you want to use fgets, you can simply overwrite the newline.

char *result = fgets(str, sizeof(str), stdin);
char len = strlen(str);
if(result != NULL && str[len - 1] == '\n')
{
  str[len - 1] = '\0';
}
else
{
  // handle error
}

This does assume there are no embedded NULLs. Another option is POSIX getline:

char *line = NULL;
size_t len = 0;
ssize_t count = getline(&line, &len, stdin);
if(count >= 1 && line[count - 1] == '\n')
{
  line[count - 1] = '\0';
}
else
{
  // Handle error
}

The advantage to getline is it does allocation and reallocation for you, it handles possible embedded NULLs, and it returns the count so you don't have to waste time with strlen. Note that you can't use an array with getline. The pointer must be NULL or free-able.

I'm not sure what issue you're having with scanf.

Answer 5

never use gets(), it can lead to unprdictable overflows. If your string array is of size 1000 and i enter 1001 characters, i can buffer overflow your program.

Answer 6

Try using fgets() with this modified version of your CharToInt():

int CharToInt(const char *s)
{
    int i, result, temp;

    result = 0;
    i = 0;

    while(*(s+i) != '\0')
    {
        if (isdigit(*(s+i)))
        {
            temp = *(s+i) & 15;
            result = (temp + result) * 10;
        }
        i++;
    }

    return result / 10;
}

It essentially validates the input digits and ignores anything else. This is very crude so modify it and salt to taste.

Answer 7

So I am not much of a programmer but let me try to answer your question about the scanf();. I think the scanf is pretty fine and use it for mostly everything without having any issues. But you have taken a not completely correct structure. It should be:

char str[MAX];
printf("Enter some text: ");
scanf("%s", &str);
fflush(stdin);

The "&" in front of the variable is important. It tells the program where (in which variable) to save the scanned value. the fflush(stdin); clears the buffer from the standard input (keyboard) so you're less likely to get a buffer overflow.

And the difference between gets/scanf and fgets is that gets(); and scanf(); only scan until the first space ' ' while fgets(); scans the whole input. (but be sure to clean the buffer afterwards so you wont get an overflow later on)