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

[Matheus Castanho <msc@linux.ibm.com>]

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