6
\$\begingroup\$

For a recent project of mine I had to do read input from the console in pure C with no external libraries (in other words, code I've written by myself). I don't like the standard formatted input such as scanf or std::cin in C++, I feel like having to do a dummy read to get rid of the leftover newline is not great. In C++ I can do std::string in; std::getline(std::cin, in); to read an entire line, but C doesn't have that (at least not on my system). Reading input from the console is often needed, so after several times of writing it I decided I wanted something I can write once and for all. This code is simple enough that I don't think I've left too many holes, but I'd still love to hear feedback about it so please do let me know what I can do to improve it.

#ifndef UUTIL_H
#define UUTIL_H

#include <stdbool.h>
#include <stddef.h>
#include <stdio.h>

bool getLine(FILE *src, char **dest, size_t *size);

#endif
#include <stdbool.h>
#include <stddef.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>

#include "uutil.h"

bool getLine(FILE *src, char **dest, size_t *size) {
    if (src == NULL) {
        if (size != NULL) *size = 0;
        return false;
    }

    if (dest == NULL) {
        if (size != NULL) *size = 0;
        return false;
    }

    char *data;
    size_t cap = 32;
    size_t readCount = 0;

    data = malloc(cap * sizeof(char));
    if (data == NULL) {
        *size = 0;
        return false;
    }

    while (true) {
        int readVal = fgetc(src);

        if (readVal == EOF || readVal == '\n') {
            if (readCount == cap) {
                // The buffer is full, but we still need to shove in the final NUL byte

                size_t newCap = cap + 1;

                char *newData = realloc(data, newCap * sizeof(char));
                if (newData == NULL) {
                    // Failed to resize, shove it in anyway

                    data[readCount - 1] = '\0';

                    if (size != NULL) *size = readCount;
                    return false;
                }

                cap = newCap;
                data = newData;
            }

            data[readCount] = '\0'; // Do not count the final NUL byte

            if (size != NULL) *size = readCount;
            *dest = data;
            return true;
        }

        if (readCount == cap) {
            // The buffer is full, but we still have more data to shovw in

            size_t newCap = cap * 2;

            char *newData = realloc(data, newCap * sizeof(char));
            if (newData == NULL) {
                // Failed to resize, clean up stdin and return

                data[readCount - 1] = '\0';
                if (src == stdin) while (fgetc(src) != EOF);
                if (size != NULL) *size = readCount;
                return false;
            }

            cap = newCap;
            data = newData;
        }

        data[readCount++] = readVal;
    }
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ fgets is of no use here? \$\endgroup\$
    – slepic
    Dec 4, 2022 at 11:54
  • \$\begingroup\$ @slepic Well, thanks for pointing out the existence of that function , it would've been useful for when I needed it :) Then again fgets does take in a fixed buffer, but I can do the same read and resize loop. Additionally, fgets will place a newline in the buffer if it reads an entire line, while my function doesn't, behaving more closely to C++'s std::getline. Again, nothing a little wrapper can't fix, but now that I'm here I might as well learn what I can out of it. \$\endgroup\$ Dec 4, 2022 at 12:49

2 Answers 2

2
\$\begingroup\$

let me know what I can do to improve it.

Design flaw

Somehow, the caller needs to distinguish between a call that immediately ended due to '\n' vs. EOF. Just like fgets().

Perhaps a 3-way return: EOF, 0 (failure) or 1 (success)?

Design flaw 2 (subtle)

EOF occurs due to end-of-file and input error.

  • Some input, then end-of-file should return true

  • No input and end-of-file should return EOF. (See above).

  • Any amount of input, then input error should return EOF. (See above).

Review feof() and ferror().

Something like:

if (readVal == EOF) {
  ...
  // Note: `!feof()` subtlely different than `ferror()` here.
  if (readCount == 0 || !feof(src)) return EOF;
  return 1;  
}

Lack of documentation

bool getLine(FILE *src, char **dest, size_t *size) deserves a few comments, especially how to call and what the return value means.

Allocate to the reference object size, not type

Easier to code right, review and maintain.

//data = malloc(cap * sizeof(char));
data = malloc(sizeof data[0] * cap);

Declare and initialize

Avoid leaving newly declare objects unassigned.

//char *data;
//.... some time later
//data = malloc(cap * sizeof(char));

char *data = malloc(cap * sizeof(char));

Assign a dummy

As apparently size == NULL is OK, when NULL, reassign to a dummy so later code does not need multiple if (size != NULL) ... tests

size_t dummy;
if (size == NULL) {
  size = &dummy;
}

Remove special code for reallocate on end-of-line

Simply use if (readCount + 1 == cap) { in the general loop.

Final allocation

When leaving the function, perform a final "right-size" re-allocation.

Allocation growth

newCap = cap * 2 is reasonable, yet 1) does not detect overflow for a really long input. 2) Limited to SIZE_MAX/2.

Instead, start at 31 and then newCap = cap * 2 + 1, code can then handle up to SIZE_MAX.

\$\endgroup\$
3
  • \$\begingroup\$ Re: "Somehow, the caller needs to distinguish between a call that immediately ended due to '\n' vs. EOF. Just like fgets()." That one was not a necessity for my project, but I can see how that might be important for a general usage. "As apparently size == NULL is OK" That was meant to ease my usage, sometimes I just want to wait for the enter key to be pressed, in which case I don't care about the input or its length, hence that special handling. Aside from that, good points, thanks for the review. \$\endgroup\$ Dec 5, 2022 at 9:53
  • \$\begingroup\$ @404NameNotFound I think the Assign a dummy point was mis-understood. I agree the size == NULL to ease user usage - especially in cases just like "don't care about the input or its length" is a goad idea. Yet rather than have multiple (5x in getLine()) if (size != NULL) ..., starting with a single size_t dummy; if (size == NULL) { size = &dummy; } inside the function negates the need for those 5 tests. \$\endgroup\$ Dec 5, 2022 at 10:33
  • 1
    \$\begingroup\$ Ah I see, yeah that does make sense. Thanks for the clarification. \$\endgroup\$ Dec 5, 2022 at 10:37
2
\$\begingroup\$

About the overall design:

bool getLine(FILE *src, char **dest, size_t *size);

This function is intended to be called like this:

char*  buff     = NULL;
size_t buffSize = 0;

if (getLine(STDIN, &buff, &size)) 
{
  // do something with buff

  free(buff);
}

This is all good and well, but this puts a significant burden on the code to ensure that the buffer is freed. The question is, does getLine have to be able to read arbitrary length input?

Or could we design something like this:

bool getLine(FILE *src, char *dest, size_t *size);

char   buff[1024];
size_t buffSize = 1024;

if (getLine(STDIN, &buff, &buffSize)) 
{
  // do something with buff
}

or even:

size_t getLine(FILE *src, char *dest, size_t size);

char   buff[1024];

size_t len = getLine(STDIN, &buff, 1024)
if (len > 0) 
{
  // do something with buff
}

This makes the code inside the getLine function simpler and puts less burden on the caller to make sure the memory is freed.


    if (src == NULL) {
        if (size != NULL) *size = 0;
        return false;
    }

    if (dest == NULL) {
        if (size != NULL) *size = 0;
        return false;
    }

This is a matter of personal taste, but I would change the input validation into asserts. Why? The problem is that NULL (0) is generally no better indicator if a pointer is invalid as 1. Forgetting to handle a failed malloc or calling from the wrong branch is something that should be detected during development, but no at runtime.

If you must try to detect these errors at runtime for some "security" reasons, you should take same approach as the XYZ_s functions use. That is call an error handler, that calls abort by default.


char *newData = realloc(data, newCap * sizeof(char));
if (newData == NULL) {
    // Failed to resize, clean up stdin and return

    data[readCount - 1] = '\0';
    if (src == stdin) while (fgetc(src) != EOF);
    if (size != NULL) *size = readCount;
    return false;
}

Handling allocation failures are quite difficult. Here I don't understand, why is the stdin read until EOF on allocation failure. The decision what to do after failure should be in the calling code and not this function.

Setting size and data is not a bad idea. But it is questionable how to proceed here. Does reading some and returning help the calling code? How does the calling code differentiate between this alloc failure and other issues? It may be a better idea to free the memory and set everything to NULL. Maybe using error codes could make things clearer to the calling code.


if (readCount == cap) {
    // The buffer is full, but we still need to shove in the final NUL byte
    [...]
}

This special case should be integrated into the regular growing of the buffer and not at the end. Change the condition below to (readCount + 1) == cap and remove this special handling.


if (newData == NULL) {
    // Failed to resize, shove it in anyway

    data[readCount - 1] = '\0';

    if (size != NULL) *size = readCount;
    return false;
}

I see what you are trying to do here; but as stated above out of memory handling is hard. It is questionable that the program is in a state to continue to operate for this hack to be a good idea.


Finally, as @slepic points out; fgets exists.

\$\endgroup\$
7
  • 1
    \$\begingroup\$ I can't vote up your question yet, but this answer is good. \$\endgroup\$
    – pacmaninbw
    Dec 4, 2022 at 13:49
  • 2
    \$\begingroup\$ A weakness to fgets() is the challenge to detect if the buffer contain null characters that were read. \$\endgroup\$ Dec 4, 2022 at 23:18
  • 1
    \$\begingroup\$ A short-coming to size_t len = getLine(STDIN, &buff, 1024) is how to distinguish between reading "\n" and end-of-file as they both return 0. \$\endgroup\$ Dec 5, 2022 at 0:56
  • 1
    \$\begingroup\$ "why is the stdin read until EOF on allocation failure. ... The decision what to do after failure should be in the calling code and not this function." --> Since false is returned for a number of reasons, under what condition should the calling code know that the entire line was not read and attempt read until end of line. getLine() knows, so it is reasonable for that function to clean up. \$\endgroup\$ Dec 5, 2022 at 1:00
  • 1
    \$\begingroup\$ @404NameNotFound Note that by allocating more and more memory, code does prevent buffer overflow, yet it does allow a hostile user to exhaust memory resources. IMO, having a sane upper limit is prudent. E. g. if input was a person's name, a name of hundreds of characters might be unexpected, yet legal. OTOH, not allowing a name over, say 10,000 is a DoS prevention tactic. \$\endgroup\$ Dec 5, 2022 at 10:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.