Fix stringop-overflow errors from gcc 10 in iconv.

  On s390x, I've recognize various -Werror=stringop-overflow messages
in iconv/loop.c and iconv/skeleton.c if build with gcc10 -O3.

With this commit gcc knows the size and do not raise those errors anymore.
---
 iconv/loop.c     | 14 ++++++++------
 iconv/skeleton.c |  8 +++++---
 2 files changed, 13 insertions(+), 9 deletions(-)
  

ping

On 6/16/20 2:24 PM, Stefan Liebler wrote:
> On s390x, I've recognize various -Werror=stringop-overflow messages
> in iconv/loop.c and iconv/skeleton.c if build with gcc10 -O3.
> 
> With this commit gcc knows the size and do not raise those errors anymore.
> ---
>  iconv/loop.c     | 14 ++++++++------
>  iconv/skeleton.c |  8 +++++---
>  2 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/iconv/loop.c b/iconv/loop.c
> index 9f7570d591..b032fcd9ac 100644
> --- a/iconv/loop.c
> +++ b/iconv/loop.c
> @@ -420,8 +420,10 @@ SINGLE(LOOPFCT) (struct __gconv_step *step,
>  #  else
>        /* We don't have enough input for another complete input
>  	 character.  */
> -      while (inptr < inend)
> -	state->__value.__wchb[inlen++] = *inptr++;
> +      size_t inlen_after = inlen + (inend - inptr);
> +      assert (inlen_after <= sizeof (state->__value.__wchb));
> +      for (; inlen < inlen_after; inlen++)
> +	state->__value.__wchb[inlen] = *inptr++;
>  #  endif
>  
>        return __GCONV_INCOMPLETE_INPUT;
> @@ -483,11 +485,11 @@ SINGLE(LOOPFCT) (struct __gconv_step *step,
>        /* We don't have enough input for another complete input
>  	 character.  */
>        assert (inend - inptr > (state->__count & ~7));
> -      assert (inend - inptr <= sizeof (state->__value));
> +      assert (inend - inptr <= sizeof (state->__value.__wchb));
>        state->__count = (state->__count & ~7) | (inend - inptr);
> -      inlen = 0;
> -      while (inptr < inend)
> -	state->__value.__wchb[inlen++] = *inptr++;
> +      for (inlen = 0; inlen < inend - inptr; inlen++)
> +	state->__value.__wchb[inlen] = inptr[inlen];
> +      inptr = inend;
>  #  endif
>      }
>  
> diff --git a/iconv/skeleton.c b/iconv/skeleton.c
> index 1dc642e2fc..1a38b51a5a 100644
> --- a/iconv/skeleton.c
> +++ b/iconv/skeleton.c
> @@ -795,11 +795,13 @@ FUNCTION_NAME (struct __gconv_step *step, struct __gconv_step_data *data,
>  # else
>  	  /* Make sure the remaining bytes fit into the state objects
>  	     buffer.  */
> -	  assert (inend - *inptrp < 4);
> +	  size_t cnt_after = inend - *inptrp;
> +	  assert (cnt_after <= sizeof (data->__statep->__value.__wchb));
>  
>  	  size_t cnt;
> -	  for (cnt = 0; *inptrp < inend; ++cnt)
> -	    data->__statep->__value.__wchb[cnt] = *(*inptrp)++;
> +	  for (cnt = 0; cnt < cnt_after; ++cnt)
> +	    data->__statep->__value.__wchb[cnt] = (*inptrp)[cnt];
> +	  *inptrp = inend;
>  	  data->__statep->__count &= ~7;
>  	  data->__statep->__count |= cnt;
>  # endif
>
  

Hi Stefan,

On 6/16/20 9:24 AM, Stefan Liebler via Libc-alpha wrote:
> On s390x, I've recognize various -Werror=stringop-overflow messages
> in iconv/loop.c and iconv/skeleton.c if build with gcc10 -O3.
> 
> With this commit gcc knows the size and do not raise those errors anymore.
> ---
>  iconv/loop.c     | 14 ++++++++------
>  iconv/skeleton.c |  8 +++++---
>  2 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/iconv/loop.c b/iconv/loop.c
> index 9f7570d591..b032fcd9ac 100644
> --- a/iconv/loop.c
> +++ b/iconv/loop.c
> @@ -420,8 +420,10 @@ SINGLE(LOOPFCT) (struct __gconv_step *step,
>  #  else
>        /* We don't have enough input for another complete input
>  	 character.  */
> -      while (inptr < inend)
> -	state->__value.__wchb[inlen++] = *inptr++;
> +      size_t inlen_after = inlen + (inend - inptr);
> +      assert (inlen_after <= sizeof (state->__value.__wchb));
> +      for (; inlen < inlen_after; inlen++)
> +	state->__value.__wchb[inlen] = *inptr++;
>  #  endif
>  
>        return __GCONV_INCOMPLETE_INPUT;
> @@ -483,11 +485,11 @@ SINGLE(LOOPFCT) (struct __gconv_step *step,
>        /* We don't have enough input for another complete input
>  	 character.  */
>        assert (inend - inptr > (state->__count & ~7));
> -      assert (inend - inptr <= sizeof (state->__value));
> +      assert (inend - inptr <= sizeof (state->__value.__wchb));
>        state->__count = (state->__count & ~7) | (inend - inptr);
> -      inlen = 0;
> -      while (inptr < inend)
> -	state->__value.__wchb[inlen++] = *inptr++;
> +      for (inlen = 0; inlen < inend - inptr; inlen++)
> +	state->__value.__wchb[inlen] = inptr[inlen];
> +      inptr = inend;
>  #  endif
>      }
>  
> diff --git a/iconv/skeleton.c b/iconv/skeleton.c
> index 1dc642e2fc..1a38b51a5a 100644
> --- a/iconv/skeleton.c
> +++ b/iconv/skeleton.c
> @@ -795,11 +795,13 @@ FUNCTION_NAME (struct __gconv_step *step, struct __gconv_step_data *data,
>  # else
>  	  /* Make sure the remaining bytes fit into the state objects
>  	     buffer.  */
> -	  assert (inend - *inptrp < 4);
> +	  size_t cnt_after = inend - *inptrp;
> +	  assert (cnt_after <= sizeof (data->__statep->__value.__wchb));
>  
>  	  size_t cnt;
> -	  for (cnt = 0; *inptrp < inend; ++cnt)
> -	    data->__statep->__value.__wchb[cnt] = *(*inptrp)++;
> +	  for (cnt = 0; cnt < cnt_after; ++cnt)
> +	    data->__statep->__value.__wchb[cnt] = (*inptrp)[cnt];
> +	  *inptrp = inend;
>  	  data->__statep->__count &= ~7;
>  	  data->__statep->__count |= cnt;
>  # endif
> 


Thanks for working on this! I also noticed this same issue on power.
This patch indeed fixes it there as well.

As for the patch, I'm not that familiar with iconv code, but by checking
the modified snippet, the loops seem equivalent and the pointers are
properly modified as before. So it's mostly harmless, basically
rewriting those lines in a different way to please GCC.

LGTM.

--
Matheus Castanho
  

On 6/26/20 8:00 PM, Matheus Castanho wrote:
> Hi Stefan,
> 
> On 6/16/20 9:24 AM, Stefan Liebler via Libc-alpha wrote:
>> On s390x, I've recognize various -Werror=stringop-overflow messages
>> in iconv/loop.c and iconv/skeleton.c if build with gcc10 -O3.
>>
>> With this commit gcc knows the size and do not raise those errors anymore.
>> ---
>>  iconv/loop.c     | 14 ++++++++------
>>  iconv/skeleton.c |  8 +++++---
>>  2 files changed, 13 insertions(+), 9 deletions(-)
>>
>> diff --git a/iconv/loop.c b/iconv/loop.c
>> index 9f7570d591..b032fcd9ac 100644
>> --- a/iconv/loop.c
>> +++ b/iconv/loop.c
>> @@ -420,8 +420,10 @@ SINGLE(LOOPFCT) (struct __gconv_step *step,
>>  #  else
>>        /* We don't have enough input for another complete input
>>  	 character.  */
>> -      while (inptr < inend)
>> -	state->__value.__wchb[inlen++] = *inptr++;
>> +      size_t inlen_after = inlen + (inend - inptr);
>> +      assert (inlen_after <= sizeof (state->__value.__wchb));
>> +      for (; inlen < inlen_after; inlen++)
>> +	state->__value.__wchb[inlen] = *inptr++;
>>  #  endif
>>  
>>        return __GCONV_INCOMPLETE_INPUT;
>> @@ -483,11 +485,11 @@ SINGLE(LOOPFCT) (struct __gconv_step *step,
>>        /* We don't have enough input for another complete input
>>  	 character.  */
>>        assert (inend - inptr > (state->__count & ~7));
>> -      assert (inend - inptr <= sizeof (state->__value));
>> +      assert (inend - inptr <= sizeof (state->__value.__wchb));
>>        state->__count = (state->__count & ~7) | (inend - inptr);
>> -      inlen = 0;
>> -      while (inptr < inend)
>> -	state->__value.__wchb[inlen++] = *inptr++;
>> +      for (inlen = 0; inlen < inend - inptr; inlen++)
>> +	state->__value.__wchb[inlen] = inptr[inlen];
>> +      inptr = inend;
>>  #  endif
>>      }
>>  
>> diff --git a/iconv/skeleton.c b/iconv/skeleton.c
>> index 1dc642e2fc..1a38b51a5a 100644
>> --- a/iconv/skeleton.c
>> +++ b/iconv/skeleton.c
>> @@ -795,11 +795,13 @@ FUNCTION_NAME (struct __gconv_step *step, struct __gconv_step_data *data,
>>  # else
>>  	  /* Make sure the remaining bytes fit into the state objects
>>  	     buffer.  */
>> -	  assert (inend - *inptrp < 4);
>> +	  size_t cnt_after = inend - *inptrp;
>> +	  assert (cnt_after <= sizeof (data->__statep->__value.__wchb));
>>  
>>  	  size_t cnt;
>> -	  for (cnt = 0; *inptrp < inend; ++cnt)
>> -	    data->__statep->__value.__wchb[cnt] = *(*inptrp)++;
>> +	  for (cnt = 0; cnt < cnt_after; ++cnt)
>> +	    data->__statep->__value.__wchb[cnt] = (*inptrp)[cnt];
>> +	  *inptrp = inend;
>>  	  data->__statep->__count &= ~7;
>>  	  data->__statep->__count |= cnt;
>>  # endif
>>
> 
> 
> Thanks for working on this! I also noticed this same issue on power.
> This patch indeed fixes it there as well.
> 
> As for the patch, I'm not that familiar with iconv code, but by checking
> the modified snippet, the loops seem equivalent and the pointers are
> properly modified as before. So it's mostly harmless, basically
> rewriting those lines in a different way to please GCC.
> 
> LGTM.
> 
> --
> Matheus Castanho
> 

@Carlos: Is this patch okay to commit before the release?

Bye.
Stefan
  

On 7/6/20 10:30 AM, Stefan Liebler wrote:
> On 6/26/20 8:00 PM, Matheus Castanho wrote:
>> Hi Stefan,
>>
>> On 6/16/20 9:24 AM, Stefan Liebler via Libc-alpha wrote:
>>> On s390x, I've recognize various -Werror=stringop-overflow messages
>>> in iconv/loop.c and iconv/skeleton.c if build with gcc10 -O3.
>>>
>>> With this commit gcc knows the size and do not raise those errors anymore.
>>> ---
>>>  iconv/loop.c     | 14 ++++++++------
>>>  iconv/skeleton.c |  8 +++++---
>>>  2 files changed, 13 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/iconv/loop.c b/iconv/loop.c
>>> index 9f7570d591..b032fcd9ac 100644
>>> --- a/iconv/loop.c
>>> +++ b/iconv/loop.c
>>> @@ -420,8 +420,10 @@ SINGLE(LOOPFCT) (struct __gconv_step *step,
>>>  #  else
>>>        /* We don't have enough input for another complete input
>>>  	 character.  */
>>> -      while (inptr < inend)
>>> -	state->__value.__wchb[inlen++] = *inptr++;
>>> +      size_t inlen_after = inlen + (inend - inptr);
>>> +      assert (inlen_after <= sizeof (state->__value.__wchb));
>>> +      for (; inlen < inlen_after; inlen++)
>>> +	state->__value.__wchb[inlen] = *inptr++;
>>>  #  endif
>>>  
>>>        return __GCONV_INCOMPLETE_INPUT;
>>> @@ -483,11 +485,11 @@ SINGLE(LOOPFCT) (struct __gconv_step *step,
>>>        /* We don't have enough input for another complete input
>>>  	 character.  */
>>>        assert (inend - inptr > (state->__count & ~7));
>>> -      assert (inend - inptr <= sizeof (state->__value));
>>> +      assert (inend - inptr <= sizeof (state->__value.__wchb));
>>>        state->__count = (state->__count & ~7) | (inend - inptr);
>>> -      inlen = 0;
>>> -      while (inptr < inend)
>>> -	state->__value.__wchb[inlen++] = *inptr++;
>>> +      for (inlen = 0; inlen < inend - inptr; inlen++)
>>> +	state->__value.__wchb[inlen] = inptr[inlen];
>>> +      inptr = inend;
>>>  #  endif
>>>      }
>>>  
>>> diff --git a/iconv/skeleton.c b/iconv/skeleton.c
>>> index 1dc642e2fc..1a38b51a5a 100644
>>> --- a/iconv/skeleton.c
>>> +++ b/iconv/skeleton.c
>>> @@ -795,11 +795,13 @@ FUNCTION_NAME (struct __gconv_step *step, struct __gconv_step_data *data,
>>>  # else
>>>  	  /* Make sure the remaining bytes fit into the state objects
>>>  	     buffer.  */
>>> -	  assert (inend - *inptrp < 4);
>>> +	  size_t cnt_after = inend - *inptrp;
>>> +	  assert (cnt_after <= sizeof (data->__statep->__value.__wchb));
>>>  
>>>  	  size_t cnt;
>>> -	  for (cnt = 0; *inptrp < inend; ++cnt)
>>> -	    data->__statep->__value.__wchb[cnt] = *(*inptrp)++;
>>> +	  for (cnt = 0; cnt < cnt_after; ++cnt)
>>> +	    data->__statep->__value.__wchb[cnt] = (*inptrp)[cnt];
>>> +	  *inptrp = inend;
>>>  	  data->__statep->__count &= ~7;
>>>  	  data->__statep->__count |= cnt;
>>>  # endif
>>>
>>
>>
>> Thanks for working on this! I also noticed this same issue on power.
>> This patch indeed fixes it there as well.
>>
>> As for the patch, I'm not that familiar with iconv code, but by checking
>> the modified snippet, the loops seem equivalent and the pointers are
>> properly modified as before. So it's mostly harmless, basically
>> rewriting those lines in a different way to please GCC.
>>
>> LGTM.
>>
>> --
>> Matheus Castanho
>>
> 
> @Carlos: Is this patch okay to commit before the release?

Yes, this doesn't change ABI/API and fixes compiles with gcc 10.

It is not subject to the ABI/API freeze currently in effect.

Please do continue to fix bugs and enable operation with the
latest released upstream toolchains.
  

On 7/6/20 5:03 PM, Carlos O'Donell wrote:
> On 7/6/20 10:30 AM, Stefan Liebler wrote:
>> On 6/26/20 8:00 PM, Matheus Castanho wrote:
>>> Hi Stefan,
>>>
>>> On 6/16/20 9:24 AM, Stefan Liebler via Libc-alpha wrote:
>>>> On s390x, I've recognize various -Werror=stringop-overflow messages
>>>> in iconv/loop.c and iconv/skeleton.c if build with gcc10 -O3.
>>>>
>>>> With this commit gcc knows the size and do not raise those errors anymore.
...
>>>>
>>>
>>>
>>> Thanks for working on this! I also noticed this same issue on power.
>>> This patch indeed fixes it there as well.
>>>
>>> As for the patch, I'm not that familiar with iconv code, but by checking
>>> the modified snippet, the loops seem equivalent and the pointers are
>>> properly modified as before. So it's mostly harmless, basically
>>> rewriting those lines in a different way to please GCC.
>>>
>>> LGTM.
>>>
>>> --
>>> Matheus Castanho
>>>
>>
>> @Carlos: Is this patch okay to commit before the release?
> 
> Yes, this doesn't change ABI/API and fixes compiles with gcc 10.
> 
> It is not subject to the ABI/API freeze currently in effect.
> 
> Please do continue to fix bugs and enable operation with the
> latest released upstream toolchains.
> 
Committed.

Thanks.
Stefan