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

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - libc-alpha mailing list
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
--
2.25.0

Reply | Threaded
Open this post in threaded view
|

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

Sourceware - libc-alpha mailing list
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
>

Reply | Threaded
Open this post in threaded view
|

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

Sourceware - libc-alpha mailing list
In reply to this post by Sourceware - libc-alpha mailing list
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
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - libc-alpha mailing list
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
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - libc-alpha mailing list
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.

Reply | Threaded
Open this post in threaded view
|

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

Sourceware - libc-alpha mailing list
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