Re: strstr.c:105:3: error: ‘for’ loop initial declarations are only allowed in C99 or C11 mode

[Giacomo Tesio]

Orlando, I have no issue if newlib decide to require -std=c99 but I
think the build scripts should impose it, without affecting each
single port. Otherwise the shared code should not require C99: using
C99 would be an option, but not mandatory.

As for the UB, I didn't think about it, I just fixed the compilation
error preserving everything else unchanged.


Giacomo

On 13/08/2019, Corinna Vinschen <vinschen@redhat.com> wrote:
> On Aug 12 20:18, Orlando Arias wrote:
>> On 8/12/19 8:05 PM, Howland, Craig D. - US via newlib wrote:
>> [snip]
>> >>        for (int i = 0; i < ne_len; i++)
>> >>        ^
>> [snip]
>> >
>> >
>> >      A quick fix for you is to do what the error message suggests:  add
>> > -std=gnu99 to CFLAGS before you configure Newlib, to have that as a
>> > default compiler flag when it is built.
>> >     This is probably also the best solution for the real fix in Newlib.
>> > There are other cases of C99 constructs being used, anyway.  (It has
>> > been 20 years.  Seems totally reasonable to require it.)
>> >                 Craig
>> >
>>
>> Greetings,
>>
>> There is still a lingering bug in the code above, and the proposed patch
>> in [1]. It will trigger undefined behavior due to the comparison of a
>> signed type and an unsigned type. If the value of ne_len or similar is
>> over INT_MAX, the comparison will always fail and cause an integer
>> overflow, resulting in undefined behavior [signed overflow is UB per C
>> standard]. The datatype that should be used in these iterations is
>> size_t. I do agree however that requiring C99 or newer to compile newlib
>> should be a thing.
>>
>> Cheers,
>> Orlando.
>>
>> [1] https://sourceware.org/ml/newlib/2019/msg00469.html
>>
>
> I don't see any such patch in there fixing the undefined behaviour
> you're talking about:
>
> @@ -142,6 +145,7 @@ strstr (const char *haystack, const char *needle)
>  {
>    const unsigned char *hs = (const unsigned char *) haystack;
>    const unsigned char *ne = (const unsigned char *) needle;
> +  int i;
>
>    /* Handle short needle special cases first.  */
>    if (ne[0] == '\0')
> @@ -170,7 +174,7 @@ strstr (const char *haystack, const char *needle)
>
>        /* Initialize bad character shift hash table.  */
>        memset (shift, ne_len + 1, sizeof (shift));
> -      for (int i = 0; i < ne_len; i++)
> +      for (i = 0; i < ne_len; i++)
>  	shift[ne[i] % sizeof (shift)] = ne_len - i;
>
>        do
>
> These two hunks are just the pre-C99 drop-in replacement.
> Also:
>
>     if (__builtin_expect (ne_len < 255, 1))
>       [...]
>
> So ne_len is guaranteed to be < 255 in this code snippet.  Am I missing
> something?
>
>
> Corinna
>
> --
> Corinna Vinschen
> Cygwin Maintainer
> Red Hat
>