[PATCH] iconvdata: Fix invalid pointer arithmetic in ANSI_X3.110 module

[Collin Funk]

Florian Weimer <fweimer@redhat.com> writes:

> * Collin Funk:
>
>> Florian Weimer <fweimer@redhat.com> writes:
>>
>>> The expression inptr + 1 can technically be invalid: if inptr == inend,
>>> inptr may point one element past the end of an array.
>>>
>>> ---
>>>  iconvdata/ansi_x3.110.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/iconvdata/ansi_x3.110.c b/iconvdata/ansi_x3.110.c
>>> index c5506b13b8..94e6e6b745 100644
>>> --- a/iconvdata/ansi_x3.110.c
>>> +++ b/iconvdata/ansi_x3.110.c
>>> @@ -407,7 +407,7 @@ static const char from_ucs4[][2] =
>>>  	   is also available.  */					      \
>>>  	uint32_t ch2;							      \
>>>  									      \
>>> -	if (inptr + 1 >= inend)						      \
>>> +	if (inend - inptr <= 1)						      \
>>>  	  {								      \
>>>  	    /* The second character is not available.  */		      \
>>>  	    result = __GCONV_INCOMPLETE_INPUT;				      \
>>>
>>> base-commit: 15de57024611ed6e668acbc440c5e360b0543374
>>
>> Just curious, was this found with -Wstrict-overflow (I think that is the
>> right one)? Might be worth mentioning the warning options used in the
>> commit message if so.
>
> No, there's actually a funnt story behind it.

Ah, good catch then. I thought -Wstrict-overflow would show here, but
that option is a bit unpredictable in my experience. I guess because it
depends on optimizations.

> We're moving the ANSI_X3.110 module between RPM packages in RHEL 9.
> This causes file conflicts between parallel installable debuginfo
> packages packages because the build ID symbolic link under
> /usr/lib/debug is in both packages, but points to debuginfo files in
> different locations:
>
> <https://gitlab.com/redhat/centos-stream/rpms/glibc/-/merge_requests/449#note_2920111549>
>
> Making a dummy change to the converter at the same time we move it
> between RPM packages changes the /usr/lib/debug path because it's based
> on the build ID.  So I started to look for a small bug to fix in the
> converter.  I was first wondering if we should fix the mapping for 0x23
> and 0x24 (it's strange that an U.S. character set doesn't have the
> dollar sign at the expected place, but uses a multibyte sequence for
> it), but it turns out it's hard to find authoritative information about
> the character set.  Doing the pointer comparison the wrong way is quite
> common under iconvdata/, so eventually spotted it.  It's a real bug, but
> I also hope that it makes the RPM file conflict go away once we
> backport it.

Interesting, that is strange. I've slowly been adding some multibyte
tests to GNU coreutils, so that information might give me some ideas,
thanks.

I was just looking at character sets yesterday using:

    $ locale -a | cut -s -d . -f 2 | cut -d @ -f 1 | LC_ALL=C sort -u

And also noticed that some character sets are surprisingly difficult to
find information about, e.g. pt154 and tis620.

Collin