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

[Will Dietz <w@wdtz.org>]

On Mon, May 7, 2018 at 2:28 PM, Rich Felker <dalias@libc.org> wrote:
> 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.
>

Agreed about the location of the check--particularly with a similar
check made two lines earlier!
I also was unhappy about it but ultimately left it this way for sake
of simplicity.

Presumably this goto pattern is for optimizing code size (only one
copy of code between 'revout:' and 'break;')?
I'm somewhat curious if today's compilers don't move/copy this
fragment anyway, in which case the redundant checks
will be easily discarded.  Maybe.

In favor of not hoisting the check out of the switch: keeping the
checks near the output makes
it very easy to verify the right bounds are checked (or in this case, aren't).

Given how complex the control-flow is in this function, this seems
rather valuable--
especially from the perspective of ensuring correctness across future changes.

Just some thoughts, and of course defer to you for ruling on aesthetic
preferences
and what you find easiest to read/maintain :).

~Will

> Rich