7
\$\begingroup\$

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;
}
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Excellent answer, thanks. But.. If I'm not totally mistaken, it is impossible for (*s)[nchr] = 0; to ever suffer the "Off by one bug", because when nchr (num chars) reaches maxc (and n=0), you realloc and don't break. I tested with n=0 and SZINIT 1 through valgrind just to make sure. There is no problem. chux missed that one too. If n > 0 , then n + 1 was allocated initially, so you are fine. You are correct on the realloc fail, I could add a tmp and free the original before bailing. \$\endgroup\$ – David C. Rankin May 18 '15 at 7:15
  • \$\begingroup\$ @DavidC.Rankin You're right about that. I didn't follow the logic all the way through. I'll rescind that part of the review. \$\endgroup\$ – JS1 May 18 '15 at 7:19
  • \$\begingroup\$ Your reasoning on the rest was spot on. n as a pointer for a drop-in replacement for getline, the cast and ssize_t nchr for the return. (I either cast in the body, or I cast in the return, I flipped a coin and chose to keep the return clean. The const designation on n will be added. \$\endgroup\$ – David C. Rankin May 18 '15 at 7:23
  • \$\begingroup\$ @DavidC.Rankin See my edit on why nchr should be unsigned, or rather the danger of leaving it signed. \$\endgroup\$ – JS1 May 18 '15 at 7:24
  • \$\begingroup\$ Yes, I just saw that and that is a sticky wicket I've been dancing around with the failure case being -1 as with getline. That complicates the choice of type and is the reason nchr is ssize_t. I'll play with the unsigned type and see what headway I can make. \$\endgroup\$ – David C. Rankin May 18 '15 at 7:26
1
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ Thank you Loki. The pointer to n is twofold. First to provide a drop-in replacement for getline, second, I was initially returning the allocation size, but that requires a reset of n in the caller before the next read to prevent altering the limit aspect if n > 0, I didn't consider fscanf because I was wanting to provide the ability to use a #ifdef foo to alter between fgetc and fgetc_unlocked depending on the situation, however it seems worth doing. The tmp for realloc was initially provided by a custom realloc_string I removed for the same of the post. Will include it. \$\endgroup\$ – David C. Rankin May 18 '15 at 18:56
  • \$\begingroup\$ In "easier way to write this", fscanf(stream, "\n"); reads the "end of line" and all following whites-spaces This is potentially many lines more of input. Suggest instead fgetc(stream). \$\endgroup\$ – chux May 19 '15 at 3:02
  • \$\begingroup\$ sprintf(format, "%%*%d[^\n]", size); fscanf(stream, format, buffer); is a problem if the line begins with '\n' as nothing is read into buffer. buffer is left unchanged. \$\endgroup\$ – chux May 19 '15 at 3:05
1
\$\begingroup\$
  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.

\$\endgroup\$
  • \$\begingroup\$ Well, size was intentional, if user wanted 20 char, then passing 20 in n is what he gets. I wrestled with that and I may give in and go with n-1 like fgets. That was just a what "would I want call." ssize_t accepts negative values (ssize_t getline returning -1). I would rather size_t, but then I would be casting in the return. Previous *s can be allocated or not. Initial realloc takes ownership of prior allocation. 6. is intentional. The alternative would be to return an *endptr like strtol. 7. good call, 120 was getline default. Thank you for your feedback! \$\endgroup\$ – David C. Rankin May 19 '15 at 3:24
  • \$\begingroup\$ For 'Overall' 2, are you thinking a returned flag showing characters remain or a number of characters remaining? Adding , size_t *r as the last argument and checking c != '\n && c != EOF' before return gives whether characters remain (flush loop var changed below (*n) to preserve c), User responsible for reset before next call. Created version passing size not length, still chewing on that one. Thanks again. \$\endgroup\$ – David C. Rankin May 19 '15 at 5:33
  • \$\begingroup\$ @David C. Rankin 1) I thinking reading a line that is excessive returns error condition. 2) By returning size, type ssize_t is not needed. 3) Other functions can simply remove potential trailing '\n'. \$\endgroup\$ – chux May 19 '15 at 13:34
0
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ C code that crashes when invalid pointers are passed is "normal" though. \$\endgroup\$ – 200_success May 18 '15 at 7:02
  • \$\begingroup\$ It really depends on how the code is being used. Obviously it's totally acceptable when it's guaranteed an invalid pointer won't be passed. \$\endgroup\$ – Trinick May 18 '15 at 7:05
  • \$\begingroup\$ That is a good point, one I didn't consider, because as the caller, I am responsible for insuring my inputs are valid. No difference than when I call getline itself. (now I will have to check if getline does a check on n or not). \$\endgroup\$ – David C. Rankin May 18 '15 at 7:07
  • \$\begingroup\$ If the function's contract says it will not validate the inputs, then it is up to the caller to ensure they are valid. However, adding debug assertions is still a very sane idea to help debugging. @Trinick, I suggest you expand your answer to mention this. \$\endgroup\$ – glampert May 18 '15 at 15:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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