[PATCH] mbsrtowcs: Fix bug when wn is a multiple of 4

[PATCH] mbsrtowcs: Fix bug when wn is a multiple of 4

[Michael Forney]

If wn becomes 0 after processing a chunk of 4, mbsrtowcs currently
continues on, wrapping wn around to -1, causing the rest of the string
to be processed.

This resulted in buffer overruns if there was only space in ws for wn
wide characters.
---
Hi,

I found this bug while tracking down a SIGSEGV in bash when globbing a large
pattern.

 src/multibyte/mbsrtowcs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/multibyte/mbsrtowcs.c b/src/multibyte/mbsrtowcs.c
index b9bbc33..c5a30de 100644
--- a/src/multibyte/mbsrtowcs.c
+++ b/src/multibyte/mbsrtowcs.c
@@ -66,6 +66,7 @@ resume0:
 				*ws++ = *s++;
 				wn -= 4;
 			}
+			if (!wn) continue;
 		}
 		if (*s-1u < 0x7f) {
 			*ws++ = *s++;
-- 
1.8.4

Re: [PATCH] mbsrtowcs: Fix bug when wn is a multiple of 4

[Rich Felker]

On Fri, Sep 27, 2013 at 01:54:42AM -0700, Michael Forney wrote:
> If wn becomes 0 after processing a chunk of 4, mbsrtowcs currently
> continues on, wrapping wn around to -1, causing the rest of the string
> to be processed.
> 
> This resulted in buffer overruns if there was only space in ws for wn
> wide characters.
> ---
> Hi,
> 
> I found this bug while tracking down a SIGSEGV in bash when globbing a large
> pattern.

Thanks! That's a nice find.

>  src/multibyte/mbsrtowcs.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/multibyte/mbsrtowcs.c b/src/multibyte/mbsrtowcs.c
> index b9bbc33..c5a30de 100644
> --- a/src/multibyte/mbsrtowcs.c
> +++ b/src/multibyte/mbsrtowcs.c
> @@ -66,6 +66,7 @@ resume0:
>  				*ws++ = *s++;
>  				wn -= 4;
>  			}
> +			if (!wn) continue;

Rather than adding an extra branch here, why not just either change
the >=4 condition to >=5 or unconditionally continue here? Any
thoughts on what would be better?

Rich

Re: [PATCH] mbsrtowcs: Fix bug when wn is a multiple of 4

[Rich Felker]

On Fri, Sep 27, 2013 at 11:28:49AM -0400, Rich Felker wrote:
> >  src/multibyte/mbsrtowcs.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/multibyte/mbsrtowcs.c b/src/multibyte/mbsrtowcs.c
> > index b9bbc33..c5a30de 100644
> > --- a/src/multibyte/mbsrtowcs.c
> > +++ b/src/multibyte/mbsrtowcs.c
> > @@ -66,6 +66,7 @@ resume0:
> >  				*ws++ = *s++;
> >  				wn -= 4;
> >  			}
> > +			if (!wn) continue;
> 
> Rather than adding an extra branch here, why not just either change
> the >=4 condition to >=5 or unconditionally continue here? Any
> thoughts on what would be better?

Forget what I said about just continuing; it would lead to an infinite
loop. I think checking for wn>=5 is probably the best solution to
avoid extra branches in a fairly common code path (ASCII at an aligned
position but not 4 ASCII characters in a row). At some point perhaps
all of this code should be reworked (with proper benchmarking to
measure the effect of changes) but for now we just need a fix.

Rich

Re: [PATCH] mbsrtowcs: Fix bug when wn is a multiple of 4

[Michael Forney]

On Fri, 27 Sep 2013 11:56:18 -0400, Rich Felker <dalias@aerifal.cx> wrote:
> > Rather than adding an extra branch here, why not just either change
> > the >=4 condition to >=5 or unconditionally continue here? Any
> > thoughts on what would be better?
> 
> Forget what I said about just continuing; it would lead to an infinite
> loop. I think checking for wn>=5 is probably the best solution to
> avoid extra branches in a fairly common code path (ASCII at an aligned
> position but not 4 ASCII characters in a row). At some point perhaps
> all of this code should be reworked (with proper benchmarking to
> measure the effect of changes) but for now we just need a fix.
> 
> Rich

Yeah, I didn't make any performance considerations, I just figured this
would be the easiest/simplest fix. The solution you've committed is
fine. Thanks!

-- 
Michael Forney <mforney@mforney.org>