Re: my strstr is broken

Re: my strstr is broken

[Wilco Dijkstra]

Adhemerval wrote:
> On 11/09/2018 15:46, Wilco Dijkstra wrote:
>> Siddhesh wrote:
>> 
>>> Is this a non-x86 architecture?  I can reproduce this on the glibc
>>> 2.28 tag when I hack the ifunc resolver to ignore
>>> __strstr_sse2_unaligned and always use the stock __strstr.
>> 
>> It's the generic strstr indeed. The issue is due to the long needle code using
>> the same AVAILABLE macro as for short needles. Since the macro now 
>> ensures the haystack contains up to 512 valid characters, it cannot handle
>> needles which are longer than this. The fix is easy, just add the needle length.
>> I'll post a patch.
>
> Does it affect memmem as well? It would be useful to check it the tests
> are inadequate and fix them if the case for memmem.

Strcasestr uses the same readahead mechanism, but memmem is fine since it
doesn't use null-terminated strings. The issue is due to trying to avoid calling strlen
on the whole string - it's not obvious that's worth the increase in complexity.

Wilco

Re: my strstr is broken

[Adhemerval Zanella]

On 11/09/2018 15:46, Wilco Dijkstra wrote:
> Siddhesh wrote:
> 
>> Is this a non-x86 architecture?  I can reproduce this on the glibc
>> 2.28 tag when I hack the ifunc resolver to ignore
>> __strstr_sse2_unaligned and always use the stock __strstr.
> 
> It's the generic strstr indeed. The issue is due to the long needle code using
> the same AVAILABLE macro as for short needles. Since the macro now 
> ensures the haystack contains up to 512 valid characters, it cannot handle
> needles which are longer than this. The fix is easy, just add the needle length.
> I'll post a patch.

Does it affect memmem as well? It would be useful to check it the tests
are inadequate and fix them if the case for memmem.

> 
>> It looks like the test case is fixed on trunk, with[1], but I'll take
>> a closer look at it later to confirm.
> 
> No it still exists on trunk, but you need avoid tests with a trivial first match
> which is now handled early as a special case. Also to make it fail consistently
> the needle needs to be longer than 1024 characters. Unfortunately neither the
> tests or benchmarks ever call two_way_long_needle...
> 
> Wilco
> 

Re: my strstr is broken

[Wilco Dijkstra]

Siddhesh wrote:

> Is this a non-x86 architecture?  I can reproduce this on the glibc
> 2.28 tag when I hack the ifunc resolver to ignore
> __strstr_sse2_unaligned and always use the stock __strstr.

It's the generic strstr indeed. The issue is due to the long needle code using
the same AVAILABLE macro as for short needles. Since the macro now 
ensures the haystack contains up to 512 valid characters, it cannot handle
needles which are longer than this. The fix is easy, just add the needle length.
I'll post a patch.

> It looks like the test case is fixed on trunk, with[1], but I'll take
> a closer look at it later to confirm.

No it still exists on trunk, but you need avoid tests with a trivial first match
which is now handled early as a special case. Also to make it fail consistently
the needle needs to be longer than 1024 characters. Unfortunately neither the
tests or benchmarks ever call two_way_long_needle...

Wilco

Re: my strstr is broken

[Siddhesh Poyarekar]

On Tuesday 11 September 2018 08:51 PM, Paul Pluzhnikov wrote:
> This is the second time in my (short) memory strstr got broken with
> very long inputs
> (https://sourceware.org/bugzilla/show_bug.cgi?id=12092).
> There was also https://sourceware.org/bugzilla/show_bug.cgi?id=14602
> 
> It seems that testing of strstr is entirely inadequate: (AFAICT) we
> only test the IFUNC-selected version, and only on small set of inputs.

I'm not sure about the assertion about not testing IFUNC versions (since 
only x86_64 and powerpc seem to be using them and their IFUNC_IMPL 
macros look correct to me and should trigger all variants provided the 
hardware supports it) but a stress test would be most welcome.

Thanks,
Siddhesh

Re: my strstr is broken

[Paul Pluzhnikov]

+cc libc-alpha

On Tue, Sep 11, 2018 at 7:51 AM Siddhesh Poyarekar
<siddhesh.poyarekar@gmail.com> wrote:

> The stock __strstr has a bug, which seems to have been fixed on master
> with the commit I mentioned.

This is the second time in my (short) memory strstr got broken with
very long inputs
(https://sourceware.org/bugzilla/show_bug.cgi?id=12092).
There was also https://sourceware.org/bugzilla/show_bug.cgi?id=14602

It seems that testing of strstr is entirely inadequate: (AFAICT) we
only test the IFUNC-selected version, and only on small set of inputs.

I filed https://sourceware.org/bugzilla/show_bug.cgi?id=23631


-- 
Paul Pluzhnikov