Re: [PATCH] Fix stringop-overflow errors from gcc 10 in iconv.

[Stefan Liebler <stli@linux.ibm.com>]

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