[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20151024233515.GZ8645@brightrain.aerifal.cx>
Date: Sat, 24 Oct 2015 19:35:15 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] Fix off-by-one buffer overflow in getdelim
On Sun, Oct 25, 2015 at 12:25:52AM +0200, Felix Janda wrote:
> 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.)
Are you saying you still think there is a bug, that's only triggered
when the byte from getc_unlocked causes the loop to break? I'll have
to review that but it seems plausible. Do you have any ideas for
adapting the test program to check this case?
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
