Implementing an alternative to scanf

Question

Since almost all C functions designed to get data from stdin are bad / flawed:

• gets - the less said about this the better
• scanf - no checks for buffer overflow and '\n' constantly remaining in stdin, screwing up next scanfs
• scanf_s - pretty much the same, but with buffer overflow checks
• fgets appends a '\n' to the string
• gets_s doesn't have the previous problem, but useless for other streams

I decided to write my own function that would at least be usable to read numbers from stdin:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
void *scano(char mode);

int main()
{

  // int *num = (int *) scano(sData, 'd');
   float *num = (float *)scano('f');
   printf("Half: %f", *(num)/2);



    return 0;
}

void *scano(char mode){

    char sData[20];
    fgets(sData, 20, stdin);
    *(sData + strlen(sData) - 1) = '\0'; //get rid of the '\n' before the '\0'
   switch(mode){
       case 'd':{
           int *dataI = (int *)malloc(sizeof(int));
           *dataI = atoi(sData);
           return dataI;
       }
       case 'f':{
           float *dataF = (float *)malloc(sizeof(float));
           *dataF = atof(sData);
           return dataF;
       }
       case 'D':{
           //double...
       }
   }

}

The function is of type void * because that's the only convenient way to have it return different data types.

The function is obviously unfinished for other data types, but I have some questions first:

• How can the algorithm of the function be improved?
• Don't I need to free() in every case? I'm aware allocated memory needs to be freed, but when working with lists, free() was only used to delete Nodes, when creating Nodes, no free() was called after a malloc().
• Is it fully safe? If not, how can it be made safe?

Answer 1

Q: How can the algorithm of the function be improved?

A: I'm not sure what you mean by algorithm. There are several things relating to functionality that you should improve on, however:

• Make an enum to represent what modes are accepted by your function, and use that instead of taking a char. This will greatly reduce the potential for bugs and makes you less reliant on magic "numbers" (or characters, if you will).
• You might want to remove the biggest drawback your function has compared to scanf and friends, which is lack of flexibility. The great strength of scanf is that its format string is extremely variable and can be used to extract data from a vast variety of formats. You approach currently limits you to a single char worth of format specifier, and is thus much more rigid and less adaptable. Depending on what you want from your function, this might be acceptable or even desired, but if you really want to contest with scanf, you should come up with a good idea of two on how to improve flexibility.
• Remove the hard limit on length. Why do you arbitrarily only read 20 chars? This is just barely sufficient for 64bit integers, but will quickly prove as too little as soon as you start reading different types of data (especially strings).
• Add some error reporting facility to your function. What if you tried to read an int when there was none? You might want to tell your users about this (actually, you absolutely must tell your users about this, or your user interface is quickly going to take a turn into an unpleasant direction).
• You state in you question that gets_s is useless for streams different from stdin, but that criticism also applies to your function. Consider taking a stream handle as your first argument and reading from that instead. You can still provide another function which has stdin implied, for convenience's sake.

---

Q: Don't I need to free() in every case?

A: Yes, you absolutely do. Every buffer that allocated via malloc and friends should be freed at some point, otherwise you've got yourself a memory leak. Actually, you might not even want to allocate things on the heap at all. If all I want to do is read a single integer, I usually don't want to incur the cost of calling out to the OS to reserve some memory for me, when I could just read the value into a local variable. How do you do this? Easy enough, instead of taking care of allocation yourself, hand that task over to the user and just accept a pointer instead, i.e.

void scano(char mode, void* buffer);

This has the added benefit of opening up the return value slot, so now you can actually use that to indicate whether an error occured:

int scano(char mode, void* buffer);

or even better

err_code scano(char mode, void* buffer);

where err_code is an enum containing all the possible error values. This pattern of returning an error indicator is really common throughout the standard library and most C applications in general.

---

Q: Is it fully safe? If not, how can it be made safe?

A: That depends on your definition of "fully safe". If you mean "has no undefined behavior", then the answer is no, sadly. atoi causes such undefined behavior in the case that the integer it is trying to parse is out of range for the int type. The same is true for atof, so you would need to brace against those cases (by not using those two functions, usually). Another problem is that you don't check for malloc failure - if allocation fails, you get a null pointer and subsequently try to write the result of atoi or atof to it, which also causes undefined behavior. Another point of insecurity is your call to fgets: If reading fails completely, your array remains uninitialized. The subsequent call to strlen is then undefined behavior.

---

Other things that aren't quite right

1. You don't need to cast the return value of malloc, or any other function returning void*. In fact, this can lead to subtle bugs when making changes to your code later on.
2. Don't rely on magic numbers. Instead of typing fgets(sData, 20, stdin), pull that 20 out to its own constant, or, at the very least, do something like fgets(sData, sizeof buf, stdin). That way, you're way less prone to buffer overflow bugs arising from you forgetting to change one of the two 20s when you go back and edit your code.
3. *(sData + strlen(sData) - 1) = '\0'; //get rid of the '\n' before the '\0' ...and now you've overwritten part of your data! There might not actually be an '\n' there, if the line you are reading from has still 19 or more characters left. Check for those kinds of things before writing into your string!

Answer 2

While I understand why you're returning a void * and mallocing, it's a total waste. There's no need to make space on the heap for types that can be returned. Additionally, your scano has the same problem that scanf has in that it doesn't do error reporting properly. scanf does some (although there are some critical cases where it doesn't handle errors properly), but yours does none. What happens if you pass a bad mode in? What if the malloc fails? What if the input number can't be parsed? What if there are multiple newlines? You handle none of these cases. So by any reasonable definition, I'd say no, your implementation is not safe.

Instead of heap allocations I'd recommend this:

int scano(char mode, void *result) {
    char buf[20];
    fgets(buf, sizeof(buf), stdin); // use sizeof, in case size changes

    // Replace first non numeric character with NULL byte
    for (size_t i = 0; i < sizeof(buf); i++) {
        if (!('0' <= buf[i] && buf[i] <= '9' | buf[i] != '.' || buf[i] == '-')) {
            buf[i] = '\0';
            break;
        }
    }

    switch (mode) {
        case 'd':
            *((int *)result) = atoi(buf);
            return 0;
        // ...

        default:
            return EINVAL;
    }
}

This has the advantage of no longer doing heap allocations (removing an edge case failure) and allowing you to report error/success. Now you can use it like so:

float num;
if (scano('f', &num) == 0) {
    // success
} else {
    // failed
}

Now this still isn't safe, because you could pass a float * with mode 'D', but that's something you can't prevent in C (C++ templates can do this).

There still are a few issues though. First, what if a number is more than 20 characters? What you really want is to progressively read from stdin until you encounter a character than can't be part of a number (maybe that's a space, maybe that's a letter unless you accept, say, base 16 encoded numbers).

The other issue is you don't handle the error case when a number is either malformed or doesn't fit in the type. atoi and friends won't do this for you. Instead look at these guides on how to properly parse numbers in C:

• https://stackoverflow.com/a/2729534/568785
• http://www.cplusplus.com/reference/cstdlib/strtod/

Answer 3

Is it fully safe? If not, how can it be made safe?

Definitely not.

atoi and atof

Using atoi and atof is per definition unsafe, since there is no way to know if the conversion succeeded or not. The functions has undefined behavior if the conversion failed.

Hard coded length

You have specified the length of a number to 20. What happens if I enter the number 123456789012345678901? You will convert it to a number, but not the intended number.

(Apart from that, for fgets(sData, 20, stdin) you have to declare sData to have a size of at least 21 if you want to treat it like a regular C-string.

Don't I need to free() in every case?

Well, no. Not inside scano. But it has to be a call to free for every call to scano. Here is some proper sample code using your implementation of scano:

float *f = scano('f');
printf("You entered the number %f\n", *f);
free(f); // If you are sure that you are done with f

Of course, you could do something like this:

float *ptr = scano('f');
float f = *ptr;
free(ptr);

But that feels very tedious if I use the function a lot. I would probably write this wrapper:

float scanofwrapper() {
    float *f = scano('f');
    float ret = *f;
    free(f);
    return ret;
}

or this, assuming that you return a null pointer on error:

int scanofwrapper(float *f) {
    float *p = scano('f');
    if(!p) return 0;
    *f = *p;
    free(p);
    return 1;
}

How can the algorithm of the function be improved?

A function like this seems pretty strange to code in a way so that you cannot use it on regular variables. In the typical case, you would like to do this: float x=scano('f'). Sure, returning a pointer gives you the benefit of returning a null pointer on error, but that's not enough to weigh up the drawbacks of being unable to use variables instead of pointers. And then I have not even touched the subject of having to use free all the time. I think you should get rid of the mallocs. Instead, let the prototype be something like:

int scano(char mode, void * data)

On success, store the read value in data. Use the return value to distinguish between success and failure. It should be used like this:

float f;
if( scano('f', &f))
    printf("You entered the number %f\n", f);

Answer 4

fgets appends a '\n' to the string

fgets() does not append '\n'. It retains a '\n' if read and then stops further reading. fgets() does append '\0'.

scanf - no checks for buffer overflow and '\n' constantly remaining in stdin, screwing up next scanfs

Widths can be applied to prevent buffer overflow. Still fgets() and then parsing with sscanf(), strtol(), etc. is better.

Is it fully safe? If not, how can it be made safe?

Code is exploitable. Consider if the first charter read is a null character. With Input like Ctrl @ x y x Enter, then sData has "\0xyz\n\0" in it. The strlen(sData) - 1 --> SIZE_MAX and *(sData + SIZE_MAX) is well outside sData --> undefined behavior (UB).

Further, code does not know the last character read is a \n' as the line could be long or the last from stdin.

Instead use

if (fgets(sData, sizeof sData, stdin) == NULL) {
  Handle_EOF_or_Input_Error();
} else {
  sData[strcspn(sData, "\n")] = '\0';  // lop off potential trailing \n
  ....
}

char sData[20];

sData[] is not so generous for reading an int and even way too conservative for reading a double. Consider a 64-bit int would need char sData[22]; to read INT_MIN and its following '\n.

double, as a string can be all over the place with hundreds of trailing 0's or leading 0.0000..... Here I would use BUFSIZ (it is at least 256). Values above that may not work well with fgets()` anyways.

How can the algorithm of the function be improved?

To improve its presentation and maintenance:

Remove unnecessary casts

// float *num = (float *)scano('f');
float *num = scano('f');

int *dataI = (int *)malloc(sizeof(int));
int *dataI = malloc(sizeof(int));

Allocate to the size of the referenced object, not type.

Easier to code right, review and maintain.

// int *dataI = malloc(sizeof(int));
int *dataI = malloc(sizeof *dataI);

'\n' constantly remaining in stdin, screwing up next scanfs

Gentle extensions to the *scanf() format would add "% " "%\n" specifiers:

1) "% " consume all non-'\n' white-spaces. Never fails. Does not add to return value.
2) "%\n" consume up to 1 '\n'. Never fails. Does not add to return value.

scanf - no checks for buffer overflow

Require a width for "%s" "%[".

To allow for maximum compatibility with scanf(), add a flag to indicate a size_t parameter exists with "%s", "%[", "%c".

char buf[10];
char ch;
my_scanf("%9s", buf);
my_scanf("%#s", sizeof buf, buf);
my_scanf("%#c", sizeof c, &c);

my_scanf("%s", buf);  // fail this form.
my_scanf("%[0-9]", buf);  // fail this form.
my_scanf("%c", &ch); // OK

Answer 5

I think if I were going to do something along this line, I would design it as close to scanf as possible, and still fix the problems cited.

At least based on what you've said, that would mean requiring that a string conversion (%s) or scanset conversion (%[...]) include either a width or precision (which are optional for the current scanf design).

The second thing I'd do would be to change an asterisk as either the width or precision in a scanf format string to work just like it does with printf, taking the next parameter to represent that value. Then I'd use a different character (e.g., @) to mean what an asterisk currently does (read this value, but don't assign it anywhere). Strictly speaking, this probably isn't required to meet your stated goals, but it turns out to be enough of a pain with scanf that it's worth fixing while you're changing things.

That would give the safety you want and retain compatibility with most existing code. The only existing code that would require any modification would be the (relatively few) that use * to mean "read but don't assign a value here" (but allowing a field width to be specified with a variable in a way that's symmetrical with printf would be worth that pain).