Re: [PATCH] strbuf: clear errno before calling getdelim(3)

From: René Scharfe <l.s.r@web.de>
Date: Aug 12, 2017, 11:57 UTC
Message-ID: <510f566b-3aee-98fd-9663-d97c3dcfeb96@web.de>
In-Reply-To: <20170812100252.bhbgg5jjlom7dfl2@ruderich.org>

Am 12.08.2017 um 12:02 schrieb Simon Ruderich:

Show 30 quoted lines

> On Fri, Aug 11, 2017 at 10:52:51AM +0200, René Scharfe wrote:
>> Am 11.08.2017 um 09:50 schrieb Simon Ruderich:
>>> On Thu, Aug 10, 2017 at 10:56:40PM +0200, René Scharfe wrote:
>>>> getdelim(3) returns -1 at the end of the file and if it encounters an
>>>> error, but sets errno only in the latter case.  Set errno to zero before
>>>> calling it to avoid misdiagnosing an out-of-memory condition due to a
>>>> left-over value from some other function call.
>>>
>>> getdelim(3p) doesn't explicitly forbid changing the errno on EOF:
>>>
>>>       If no characters were read, and the end-of-file indicator for
>>>       the stream is set, or if the stream is at end-of-file, the
>>>       end-of-file indicator for the stream shall be set and the
>>>       function shall return −1. If an error occurs, the error
>>>       indicator for the stream shall be set, and the function shall
>>>       return −1 and set errno to indicate the error.
>>
>> [snip]
>>
>>> I don't think that it matters in practice, but the "most" correct
>>> way to handle this would be to check if feof(3) is true to check
>>> for the non-errno case.
>>
>> Only if errors at EOF are guaranteed to be impossible.  Imagine
>> getdelim being able to read to the end and then failing to get memory
>> for the final NUL.  Other scenarios may be possible.
> 
> Good point. Instead of feof(3), checking ferror(3) should handle
> that properly. It's guaranteed to be set by getdelim for any
> error.

Only if we have a POSIXly correct implementation of getdelim(3). On Linux its manpage doesn't mention the error indicator, but says that the function started out as a GNU extension before becoming standardized, so I'm not sure we can depend on that everywhere.

But we probably want to check for other errors. They look unlikely enough that we may get away with something like this:

	-	if (errno == ENOMEM)
	-		die("Out of memory, getdelim failed");
	+	if (errno || ferror(fp))
	+		die_errno(_("getdelim failed"));

NB: The other errors are EINVAL (input pointers are NULL or the
stream is invalid) and EOVERFLOW (read more than fits into ssize_t)
according to POSIX and the Linux manpage.

Update: Just noticed that on the BSDs getdelim(3) doesn't set errno
to ENOMEM on allocation failure, but does set the error indicator.
That might be an oversight on their part, but that means we
certainly *need* to check ferror().  And an unchanged errno can
mean ENOMEM there.  Ugh..

> An edge case is if the error indicator was already set before
> calling getdelim() and the errno was already set to ENOMEM. This
> could be fixed by checking ferror() before calling getdelim, but
> I'm not sure if that's necessary:

Treating an error as end-of-file is not a good idea. An invalid stream should not be passed to strbuf_getwholeline() in the first place. We could call die() or BUG(), but I think we may leave that check to getdelim(3), unless we learn about an implementation that is unprepared to reject streams with the error indicator set.

René