From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/8764 Path: news.gmane.org!not-for-mail From: Felix Janda Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] Fix off-by-one buffer overflow in getdelim Date: Sun, 25 Oct 2015 00:25:52 +0200 Message-ID: <20151024222552.GA6890@nyan> References: <20151024204339.GA1352@nyan> <20151024213645.GU8645@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1445725692 29840 80.91.229.3 (24 Oct 2015 22:28:12 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 24 Oct 2015 22:28:12 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-8777-gllmg-musl=m.gmane.org@lists.openwall.com Sun Oct 25 00:28:04 2015 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1Zq7IB-0002D7-Bi for gllmg-musl@m.gmane.org; Sun, 25 Oct 2015 00:28:03 +0200 Original-Received: (qmail 20147 invoked by uid 550); 24 Oct 2015 22:28:01 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 20121 invoked from network); 24 Oct 2015 22:28:00 -0000 Mail-Followup-To: musl@lists.openwall.com Content-Disposition: inline In-Reply-To: <20151024213645.GU8645@brightrain.aerifal.cx> User-Agent: Mutt/1.5.23 (2014-03-12) Xref: news.gmane.org gmane.linux.lib.musl.general:8764 Archived-At: 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 > > I think you're mistaken. i+k is the space needed so far in the buffer > (not counting the terminating null byte) and *n is the usable space. > The equality case of the i+k >= *n conditional covers the need to > expand the buffer when the new content of length k would exactly fit > but would not leave room for null termination. > > Just to make sure I wrote a quick test program, which I've attached, > that should crash in free if the overflow occurs. It does not crash > and the output demonstrates correct resizing. Thanks for the test program! I did not see the 'if (z) break;'. The off-by-one should only occur when memchr returns 0 but the byte from getc_unlocked is the delimiter. (This makes it not so easy to observe the bug.) Felix