bug in optimised strstr

[Jeff Johnston]

Howland Craig D (Craig) wrote:
> Sam:
>      A potentially quick way of checking for Eric's 64/32-bit question
> (in
> str-two-way.h):
> -      b = CANON_ELEMENT (needle[max_suffix + k]);
> +      b = CANON_ELEMENT (needle[(size_t)(max_suffix + k)]);
>  
> This might be a good thing to do as a patch just in case.  (Since
> pointer[index] is effecively changed by compilers (per the std) to
> *((pointer) + (index)), the result of the expression is the type of the
> pointer operand.  Since pointers are not guaranteed to fit into any
> of the integer types, the cast is safest--even if not normal practice.)
>      Hey, Jeff, what exact file did you change?  I've checked my local
> CVS copy against the repository and am not seeing any changes.
> 				Craig Howland
>
>   
Thanks for the patch idea. I'll apply it.

I changed libc/sys/linux/include/stdint.h and 
libc/sys/linux/sys/stdint.h. They are only used to build x86-linux.

-- Jeff J.
> -----Original Message-----
> From: newlib-owner@sourceware.org [mailto:newlib-owner@sourceware.org]
> On Behalf Of Jeff Johnston
> Sent: Thursday, October 02, 2008 1:35 PM
> To: Eric Blake
> Cc: newlib@sources.redhat.com; Sam Clegg
> Subject: Re: bug in optimised strstr
>
> Jeff Johnston wrote:
>   
>> Eric Blake wrote:
>>     
>>> Jeff Johnston <jjohnstn <at> redhat.com> writes:
>>>
>>>  
>>>       
>>>>   max_suffix = SIZE_MAX;
>>>>   j = 0;
>>>>   k = p = 1;
>>>>   while (j + k < needle_len)
>>>>     {
>>>>       a = CANON_ELEMENT (needle[j + k]);
>>>>       b = CANON_ELEMENT (needle[max_suffix + k]);
>>>>
>>>> it is the line b=....
>>>>
>>>> It cannot be correct as you are trying to reference SIZE_MAX + 1 the
>>>>         
>
>   
>>>> first time through the loop.
>>>>     
>>>>         
>>> But the comments state:
>>>
>>>   /* Invariants:
>>>      0 <= j < NEEDLE_LEN - 1
>>>      -1 <= max_suffix{,_rev} < j (treating SIZE_MAX as if it were 
>>> signed)
>>> ...
>>>
>>> On cygwin, this works (in other words, I'm not reproducing the 
>>> crash).  The intent is that this line is referencing needle[0].  What
>>>       
>
>   
>>> type is size_t on your platform, and the value of SIZE_MAX?  Could it
>>>       
>
>   
>>> be that there is some type promotion going on, where the result of 
>>> SIZE_MAX+1 results in a 64-bit type containing 2**32 instead of 0, as
>>>       
>
>   
>>> is required by modulo math since size_t is unsigned?
>>>
>>>   
>>>       
>> Ok, I got mixed up on what SIZE_MAX was supposed to be.  For x86, 
>> stdint.h is being overridden with one in libc/sys/linux/include that 
>> has a wrong value for SIZE_MAX (LONG_MAX).  I am rebuilding now.  I 
>> can't say what is happening for arm since it should be using the same 
>> stdint.h from libc/include.
>>
>> -- Jeff J.
>>
>>
>>     
> Patch works for x86-linux and is checked in.  I don't have an arm system
>
> to play around with.  Sam, are you running arm-linux (which isn't in 
> newlib)?  That would use the code in question.  Otherwise, can you 
> provide more details (e.g. run under gdb and print out the various 
> information above)?
>
> -- Jeff J.
>
>