From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/13274 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] Fix off-by-one buffer overflow in getdelim Date: Sun, 16 Sep 2018 19:32:42 -0400 Message-ID: <20180916233242.GD17995@brightrain.aerifal.cx> References: <20151024204339.GA1352@nyan> <20180916182542.GB17995@brightrain.aerifal.cx> <20180916183447.GC17995@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: blaine.gmane.org 1537140656 26592 195.159.176.226 (16 Sep 2018 23:30:56 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sun, 16 Sep 2018 23:30:56 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-13290-gllmg-musl=m.gmane.org@lists.openwall.com Mon Sep 17 01:30:52 2018 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1g1gV6-0006lw-2O for gllmg-musl@m.gmane.org; Mon, 17 Sep 2018 01:30:48 +0200 Original-Received: (qmail 24122 invoked by uid 550); 16 Sep 2018 23:32:57 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 24099 invoked from network); 16 Sep 2018 23:32:55 -0000 Content-Disposition: inline In-Reply-To: <20180916183447.GC17995@brightrain.aerifal.cx> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:13274 Archived-At: On Sun, Sep 16, 2018 at 02:34:47PM -0400, Rich Felker wrote: > On Sun, Sep 16, 2018 at 02:25:42PM -0400, Rich Felker wrote: > > On Sat, Oct 24, 2015 at 10:43:39PM +0200, Felix Janda wrote: > > > when deciding whether to resize the buffer, the terminating null byte > > > was not taken into account > > > --- > > > src/stdio/getdelim.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/stdio/getdelim.c b/src/stdio/getdelim.c > > > index a88c393..3077490 100644 > > > --- a/src/stdio/getdelim.c > > > +++ b/src/stdio/getdelim.c > > > @@ -27,7 +27,7 @@ ssize_t getdelim(char **restrict s, size_t *restrict n, int delim, FILE *restric > > > for (;;) { > > > z = memchr(f->rpos, delim, f->rend - f->rpos); > > > k = z ? z - f->rpos + 1 : f->rend - f->rpos; > > > - if (i+k >= *n) { > > > + if (i+k+1 >= *n) { > > > if (k >= SIZE_MAX/2-i) goto oom; > > > *n = i+k+2; > > > if (*n < SIZE_MAX/4) *n *= 2; > > > -- > > > 2.4.9 > > > > This patch raised a potential conformance issue, that by a strict > > reading of the spec, getdelim is only permitted to realloc if the > > caller-provided buffer length is insufficient: > > > > "If *lineptr is a null pointer or if the object pointed to by > > *lineptr is of insufficient size, an object shall be allocated as > > if by malloc() or the object shall be reallocated as if by > > realloc(), respectively, ..." > > > > I'm going to change the +1 to +!z and add a comment. The idea is that > > the +1 was only needed in order for the result to fit if the delimiter > > has not already been found; if the memchr found it, an exact-sized > > buffer was being expanded unnecessarily. > > > > I'm replying to this thread and CC'ing in case there are any problems > > I'm missing in my new fix. > > This fix actually looks insufficient; it doesn't fix the case where > the getc produces EOF rather than a character. OK, the problem here is actually a lot more fundamental than I realized. If you read the standard as disallowing realloc unless it's necessary for the result to fit, then there's a circular dependency here. You can't realloc without knowing whether the next getc will succeed, but you can't getc without knowing there'll be at least 2 additional bytes to store the result and the null terminator. If you could fit one additional byte without allocating, but not two, there's no way to proceed. The only way out I see is to do the first reallocation speculatively: instead of realloc, malloc a new buffer that will be large enough, attempt the getc, and then either switch to it (freeing the original buffer) or free it (keeping the original buffer) depending on whether EOF is returned. In almost all cases, this logic can be skipped. It's not necessary at all if the stdio stream is buffered, since we can just unget back. (Using unget works mechanically for unbuffered streams too, but it violates the invariant that no interface except ungetc or scanf families should leave logical FILE position not equal to underlying open file descriptor's offset). It's also not necessary for additional growth after the first time, since enlarging is already committed. Rich