C - How to Read String Lines from Stdin or File Memory Save

Question

I need a version of read line that is memory save. I have this "working" solution. But I'm not sure how it behaves with memory. When I enable free(text) it works for a few lines and then I get an error. So now neither text nor result is ever freed although I malloc text. Is that correct ? And why is that so ?

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

char* readFromIn()
{
    char* text = malloc(1024);
    char* result = fgets(text, 1024, stdin);
    if (result[strlen(result) - 1] == 10)
        result[strlen(result) - 1] = 0;
    //free(text);
    return result;
}

I have A LOT of short lines to read with this and I also need stdin to be replaceable with a FILE* handle. There is no need for me to realloc text because I have only short lines.

Answer 1

fgets returns a pointer to the string, so after the fgets line, result will be the same memory address as text. Then when you call free (text); you are returning invalid memory.

You should free the memory in the calling function when you have finished with result

You could also avoid the malloc/free stuff by structuring your code to pass a buffer something like this:

void parent_function ()
{
    char *buffer[1024];

    while (readFromIn(buffer)) {
        // Process the contents of buffer
    }
}

char *readFromIn(char *buffer)
{
    char *result = fgets(buffer, 1024, stdin);
    int len;

    // fgets returns NULL on error of end of input,
    // in which case buffer contents will be undefined
    if (result == NULL) {
        return NULL;
    }

    len = strlen (buffer);
    if (len == 0) {
        return NULL;
    }

    if (buffer[len - 1] == '\n') {
        buffer[len - 1] = 0;

    return buffer;
}

Trying to avoid the malloc/free is probably wise if you are dealing with many small, short lived items so that the memory doesn't get fragmented and it should faster as well.

Answer 2

char *fgets(char *s, int size, FILE *stream) reads in at most one less than size characters from stream and stores them into the buffer pointed to by s. Reading stops after an EOF or a newline. If a newline is read, it is stored into the buffer. A terminating null byte ('\0') is stored after the last character in the buffer.

Return Value: returns s on success, and NULL on error or when end of file occurs while no characters have been read.

So there are 2 critical problems with your code:

1. You don't check the return value of fgets
2. You want to deallocate the memory, where this string is stored and return a pointer to this memory. Accessing the memory, where such a pointer (dangling pointer) points to, leads to undefined behaviour.

Your function could look like this:

public char* readFromIn() {
    char* text = malloc(1024);
    if (fgets(text, 1024, stdin) != NULL) {
        int textLen = strlen(text);
        if (textLen > 0 && text[textLen - 1] == '\n')
            text[textLen - 1] == '\0';     // getting rid of newline character
        return text;
    }
    else {
        free(text);
        return NULL;
    }
}

and then caller of this function should be responsible for deallocating the memory that return value of this function points to.

Answer 3

I know you mentioned that the lines are only short, but none of the solutions provided will work for lines greater than 1024 in length. It is for this reason that I provide a solution which will attempt to read entire lines, and resize the buffer when there's not enough space.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define MINIMUM_CAPACITY 16

size_t read_line(char **buffer, size_t *capacity) {
    char *buf = *buffer;
    size_t cap = *capacity, pos = 0;

    if (cap < MINIMUM_CAPACITY) { cap = MINIMUM_CAPACITY; }

    for (;;) {
        buf = realloc(buf, cap);
        if (buf == NULL) { return pos; }
        *buffer = buf;
        *capacity = cap;

        if (fgets(buf + pos, cap - pos, stdin) == NULL) {
            break;
        }

        pos += strcspn(buf + pos, "\n");
        if (buf[pos] == '\n') {
            break;
        }

        cap *= 2;
    }

    return pos;
}

int main(void) {
    char *line = NULL;
    size_t size = 0;

    for (size_t end = read_line(&line, &size); line[end] == '\n'; end = read_line(&line, &size)) {
        line[end] = '\0'; // trim '\n' off the end
        // process contents of buffer here
    }

    free(line);
    return 0;
}

An ideal solution should be able to operate with a fixed buffer of 1 byte. This requires a more comprehensive understanding of the problem, however. Once achieved, adapting such a solution would achieve the most optimal solution.

Answer 4

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

char *readFromIn(FILE *fp)
{
    char text[1024];
    size_t len;

    if (!fgets(text, sizeof text, fp)) return NULL;
    len = strlen(text);

    while (len && text[len-1] == '\n') text[--len] = 0;

    return strdup(text);
}

Answer 5

Why did no one propose to move the buffer from heap to stack ? This is my solution now:

char input[1024]; // held ready as buffer for fgets

char* readFromIn()
{
    char* result = fgets(input, 1024, stdin);
    if (result == null)
        return "";
    if (result[strlen(result) - 1] == '\n')
        result[strlen(result) - 1] = 0;
    return result;
}