[PATCH v3] libio: null terminate the buffer upon initial allocation in getdelim

[Collin Funk]

Hi Eric,

Eric Blake <eblake@redhat.com> writes:

> On Fri, Nov 28, 2025 at 05:12:23PM -0800, Collin Funk wrote:
>> Commit 33eff78c8b28adc4963987880e10d96761f2a167 caused issues in nbdkit
>> which had code similar to this to get the last line of the file:
>> 
>>     while (getline (&line, &len, fp) != -1)
>>       ;
>>     /* Process LINE.  */
>> 
>> After that commit, line[0] would be equal to '\0' instead of containing
>> the last line of the file like before that commit. A recent POSIX issue
>> clarified that the behavior before and after that commit are allowed,
>> since the contents of LINE are unspecified after -1 is returned
>> [1]. However, some programs rely on the previous behavior.
>> 
>> This patch null terminates the buffer upon getdelim/getline's initial
>> allocation. This is compatible with previous glibc versions, while also
>> protecting the caller from reading uninitialized memory if the file is
>> empty, as long as getline/getdelim does the initial allocation.
>> 
>> [1] https://www.austingroupbugs.net/bug_view_page.php?bug_id=1953
>> 
>> Suggested-by: Eric Blake <eblake@redhat.com>
>> ---
>>  libio/iogetdelim.c   |  7 +++++--
>>  libio/tst-getdelim.c | 47 +++++++++++++++++++++++++++++++++++++-------
>>  manual/stdio.texi    |  7 ++++++-
>>  3 files changed, 51 insertions(+), 10 deletions(-)
>
> LGTM
>
>> +  /* Test that we can read until -1 is returned and then access the last line
>> +     of the file.  This behavior was broken by commit
>> +     33eff78c8b28adc4963987880e10d96761f2a167 and later fixed.  */
>> +  lineptr = NULL;
>> +  linelen = 0;
>> +  TEST_VERIFY_EXIT (fwrite ("a\nb\nc\n", 1, sizeof "a\nb\nc\n" - 1, fp)
>> +                    == sizeof "a\nb\nc\n" - 1);
>
> Repetitive and harder to refactor, vs. defining the string in a macro
> or static array. But since it is on a single line, and since this is a
> test, I can live with it; no need to spin v4 on my account.

Probably better to use a variable, even if one line.

>
>> +  TEST_VERIFY_EXIT (0 <= fseeko (fp, 0, SEEK_SET));
>
> Would '0 ==' be better than '0 <='?  But even if you keep the weaker
> test here, the rest of the test will fail if fseeko() positioned the
> file anywhere other than expected, so again, not a reason to require
> v4 on my end.

The fseeko function returns 0 on success unlike lseek which turns the
offset, which is a bit confusing. I'll use "== 0" to avoid giving the
impression that it will return a positive value.

>> +  char expect[] = { 'a' - 1, '\n', '\0' };
>> +  for (int i = 0; i < 3; ++i)
>> +    {
>> +      ++expect[0];
>> +      TEST_VERIFY (getdelim (&lineptr, &linelen, '\n', fp) == 2);
>> +      TEST_VERIFY (linelen > 0);
>
> Stronger would be TEST_VERIFY (linelen > 2) (that is, on success,
> linelen MUST be larger than the result that getdelim just returned, to
> account for the trailing null byte that must be appended after the
> length of the string returned).

Yes, I agree. Good catch.

>> +      TEST_VERIFY (strcmp (lineptr, expect) == 0);
>
> Stronger might be strncmp (lineptr, expect, linelen) or even memcmp
> (lineptr, expect, linelen+1) (assuming you also check that linelen >
> strlen (expect)).

Well, linelen is the size of the buffer, so memcmp (..., linelen + 1)
would read past the end of it. Since the return value of getdelim (bytes
read) is already checked, I think strcmp is fine.

>
>> +    }
>
> The reason I mention a stronger test is that a bug in the
> implementation where line is not (re)alloced to something large enough
> could be masked if glibc wrote beyond the end of the buffer, but the
> strcmp still succeeds.
>
> But in the interest of getting this patch in, I'm okay with accepting
> the test as written.

Thanks for checking! I sent a v4 just so my changes could be double
checked [1]. I will push once it gets a Reviewed-by.

Collin

[1] https://inbox.sourceware.org/libc-alpha/623e8b82540e92df041604d1c44ba5f29a774dbc.1764734503.git.collin.funk1@gmail.com/