getline substitute that will enforce 'n' as limit of characters read

Question

Following from a discussion with others on StackOverflow concerning the issue with getline not limiting input and happily reallocating until enough space for a sizable nefarious string, I picked around at a substitute that I could put in place that mimics getline's behavior. The primary objective was to enforce a length restriction through size_t *n rather than simply updating n with the current allocation size. If n was specified as 0 no limit on input would apply.

In in the version below, n is not updated with current allocation size, but used for allocating n + 1 bytes to cover the allocation for reading n characters from stream. If n is 0, then initial allocation is set to an initial value (e.g. #define SZINIT 64, or it could easily be a default local variable in the function itself). Reading stops when n is reached, or if n=0 the lineptr is reallocated until \n or EOF.

The function is fairly self-explanatory, n is preserved unchanged in the function to serve as a flag for whether reading stops at n or whether the lineptr is reallocated. maxc is the maximum characters that can be read before reallocation. Reallocation follows the traditional 2 * current approach and maxc is updated to reflect the current allocation. If reallocation occurs, the allocation is always greater than the number of characters read which should ever prevent writing beyond the end of the buffer. If reading stops at n, the remaining characters in the input buffer are discarded.

So I'm posting here to see if there are flaws I haven't picked up on. I've tried to test as many corner cases as I could imagine (I haven't considered SZINIT being set to 0, as I just thought about that possibility). Other than that, give it a look and let me know what issues you see and whether there are glaring efficiency issues I can correct without overly complicating the code. (efficiency has not been the primary focus, but testing shows that for small strings memory allocation is better than getline (which default to initial allocation of 120 bytes), but as string length increases, getline is far better from an allocation count and speed standpoint). The DEBUG definitions were left just to provide convenience of testing, they are not intended to be part of the final.

/** getln reads up to n characters from stream into s, (if n=0 no limit).
 *  Reading continues until n characters read, (or newline or EOF if n=0). 
 *  s is initially reallocated 'n (or SZINIT, if n=0) + 1' bytes (to prevent 
 *  overrun regardless of allocation of s in the caller). If n=0, s is 
 *  reallocated as needed to read until newline or EOF. s is guaranteed to be 
 *  null-terminated and sufficiently allocated to contain all characters read. 
 *  On success, the number of characters read from input is returned, -1 otherwise. 
 */
ssize_t getln (char **s, size_t *n, FILE *stream)
{
    int c = 0;
#ifdef SZINIT
    size_t szinit = SZINIT > 0 ? SZINIT : 120;
#else
    size_t szinit = 120;
#endif
    size_t maxc = *n ? *n : szinit;
    ssize_t nchr = 0;

    if (!(*s = realloc (*s, (maxc + 1) * sizeof **s)))
        return -1;

    while ((size_t)nchr < maxc && (c = fgetc (stream)) != EOF && c != '\n')
    {
        (*s)[nchr++] = c;

        if ((size_t)nchr == maxc) 
        {
            if (*n) {
                while ((c = fgetc (stream)) != '\n' && c != EOF);
                break;
            }
            else {
#ifdef DEBUG
                printf ("  reallocating %zu to %zu\n", maxc, maxc * 2);
#endif
                if (!(*s = realloc (*s, 2 * maxc *sizeof **s)))
                    return -1;
                maxc *= 2;
            }
        }
    }
    (*s)[nchr] = 0;

    return nchr || c != EOF ? nchr : -1;
}

note: The reason for the pointer to n is twofold. First to provide a drop-in replacement for existing getline calls, and second, I was initially returning the allocation size through n, but that required a reset of n in the caller before the next read to prevent altering the limit aspect if n > 0, So, I was on the fence about whether to keep it a pointer or a normal variable. The pointer won out from the drop-in replacement standpoint, and the fact it preserves the ability to return the allocation size should that be needed in the future.

Both realloc calls originally called realloc_string below to prevent against the leak on realloc fail with the original pointer. This call was removed for purpose of the post and simply replaced with realloc. I'll provide a tmp pointer to prevent against the leak on fail in the final:

/* reallocate string */
char *realloc_string (char *s, size_t *n)
{
    char *tmp = realloc (s, 2 * *n * sizeof *s);
    if (!tmp) {
        fprintf (stderr, "%s() error: reallocation failed.\n", __func__);
        return NULL;
    }
    s = tmp;
    memset (s + *n, 0, *n * sizeof *s); /* memset new mem 0 */
    *n *= 2;

    return s;
}

Answer 1

Types

It seems that n is used only as an input value, so I would just make it a size_t instead of a pointer. If you are keeping it a pointer to make this function look like getline(), then I would suggest making it a const size_t *.

The variable nchr is strange because you declare it signed but cast it to unsigned everywhere. I assume that the only reason it is signed in the first place is because the function itself returns a signed value. In that case, I think that the function should have a maximum bytes read of SSIZE_T_MAX and nchr should not be allowed to exceed that value. Either that or change the return value to unsigned.

As it stands, if for some reason nchr wraps around and becomes negative, this line will start writing outside the buffer:

(*s)[nchr++] = c;

Possible memory leak

If any of the calls to realloc fail, the previous allocation will be lost because you will no longer have any pointer to it. I'm not sure how important this is to you though.

Answer 2

Why are you passing a pointer to n?

ssize_t getln (char **s, size_t *n, FILE *stream)

If your original s pointed at memory. Then you just leaked it if realloc failed.

if (!(*s = realloc (*s, (maxc + 1) * sizeof **s)))

The result of re-alloc should be assigned to a temporary. You only overwrite your variable on success (otherwise you just leaked the memory that was originally pointed at).

char* tmp = realloc (*s, (maxc + 1) * sizeof **s);
if (!tmp) {
    return -1;
}
*s = tmp;

Note:

while ((size_t)nchr < maxc && (c = fgetc (stream)) != EOF && c != '\n')

Returning EOF is not exclusive to eof. The fgetc() function will return EOF when there is an error. This is probably what you want. But it may bw worth checking to make sure there was not an error you want to report.

There is a much easier way to write this:

while ((c = fgetc (stream)) != '\n' && c != EOF);

// or let the stream code do it.

fscanf(stream, "%*[^\n]"); // read and ignore all values upto end of line.
fscanf(stream, "\n");      // read the end of line.

A lot of this code can be removed if you use fscanf(). As part of the format specifiers you can set a maximum input length size.

sprintf(format, "%%*%d[^\n]", size);  // builds "%*15[^\n]"
fscanf(stream, format, buffer);       // reads at most 15 (n) characters into buffer.

Answer 3

1. Passing length rather than size: This function departs from getline() which uses *n for the size of the allocation, not the maximum length of the string - which necessitates a +1 difference. This certainly will cause confusion. Recommend to use *n to represent the size of the allocation.

   ssize_t getln (char **s, size_t *n, FILE *stream)
   ssize_t getline (char **s, size_t *n, FILE *stream)
   

2. n name. *n is the maximum length, yet uses a name that hints of size. Better to use a name of max_length.

3. Make clear *s needs to be NULL or the result of a precious malloc() and afterward free(*s) needed.

4. Unclear as to why the cast (size_t) nchr. ssize_t may/may not be more capable type than size_t, so recommend nchr < maxc and nchr == maxc. See https://stackoverflow.com/q/19224655/2410359. Further ssize_t introduces a reliance outside the C standard.

5. if (!(*s = realloc (*s, 2 * maxc *sizeof **s))) may leak memory as mentioned by others - but I do see you have a realloc_string().

6. No discernment if data loss when line longer than *n.

7. Minor: no detect of overflow on maxc * 2. Suggest using size_t szinit = some_power_of_2_minus_1; rather than 120 to allow reaching values near SIZE_MAX.

8. *n == 0 with special meaning - not bad - but seems to overload the function too much. Suggest 2 separate functions.

---

Overall:

1. Much prefer using size parameters rather than length.

2. Prefer knowing if line ended in '\n' or not - which this does not provide.

3. Prefer solutions that do not use non C-standard types like ssize_t.

Answer 4

There's no null check on n prior to referencing its value. If NULL is passed as n an exception will likely be issued.