Re: [PATCH] iconv: add check to avoid writing past end of buffer

[Rich Felker <dalias@libc.org>]

On Wed, May 02, 2018 at 12:07:17PM -0500, Will Dietz wrote:
> Attached.
> 
> Example based on [1] that crashes without this fix can be found here:
> 
> https://gist.github.com/7bc07da1dcd02e01c2fbb28cbaa81420
> 
> Input is from git's tests (2.17.0), and fixes tests when using
> noxcuse-based iconv utility and musl's iconv implementation.
> 
> Well, *almost*.   At least no more crashing :).  One final test
> involving autosquash fails-- I believe due to a comparison breaking
> due to unexpected shifts in ISO-2022-JP encoding (as described in [2])
> but I'm not sure of details just yet.  Neat to get this far!
> 
> ~Will
> 
> [1] http://www.openwall.com/lists/musl/2017/05/03/1
> [2] http://www.openwall.com/lists/musl/2014/11/09/1

> From d4516bbca6b315927b82252baa24574ae12f0b06 Mon Sep 17 00:00:00 2001
> From: Will Dietz <w@wdtz.org>
> Date: Tue, 1 May 2018 14:16:44 -0500
> Subject: [PATCH] iconv.c: add missing check against output buffer size
> 
> ---
>  src/locale/iconv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/locale/iconv.c b/src/locale/iconv.c
> index d469856c..3c1f4dd2 100644
> --- a/src/locale/iconv.c
> +++ b/src/locale/iconv.c
> @@ -539,6 +539,7 @@ size_t iconv(iconv_t cd, char **restrict in, size_t *restrict inb, char **restri
>  			if (*outb < 1) goto toobig;
>  			if (c<256 && c==legacy_map(tomap, c)) {
>  			revout:
> +				if (*outb < 1) goto toobig;
>  				*(*out)++ = c;
>  				*outb -= 1;
>  				break;
> -- 
> 2.17.0
> 

This also looks correct. I'm not too fond of the location of the
check but I'm not sure there's anywhere I'd like much better. The
other options look like at the beginning of each relevant case or
before the whole switch. In some ways before the switch is nice
(eliminates lots of duplicate checks) but it's redundant in the case
of dest encodings that are always >1 byte.

Rich