Fix stringop-overflow errors from gcc 10 in iconv.

Message ID 20200616122420.5175-1-stli@linux.ibm.com
State New
Headers show
Series
  • Fix stringop-overflow errors from gcc 10 in iconv.
Related show

Commit Message

Adhemerval Zanella via Libc-alpha June 16, 2020, 12:24 p.m.
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(-)

-- 
2.25.0

Comments

Adhemerval Zanella via Libc-alpha June 26, 2020, 7:52 a.m. | #1
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

>
Adhemerval Zanella via Libc-alpha June 26, 2020, 6 p.m. | #2
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
Adhemerval Zanella via Libc-alpha July 6, 2020, 2:30 p.m. | #3
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
Adhemerval Zanella via Libc-alpha July 6, 2020, 3:03 p.m. | #4
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.

-- 
Cheers,
Carlos.
Adhemerval Zanella via Libc-alpha July 7, 2020, 7:44 a.m. | #5
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

Patch

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