Re: [PATCH] add memrchr(3)

[Christopher Faylor <cgf-use-the-mailinglist-please at sourceware dot org>]

On Wed, May 09, 2012 at 08:23:52AM -0600, Eric Blake wrote:
>On 05/09/2012 03:01 AM, Corinna Vinschen wrote:
>>> {
>>>   _CONST _PTR src_end = (const unsigned char *) src + length - sizeof (unsigned char);
>
>sizeof(unsigned char) is defined by C to be exactly 1; I always question
>code that spells it out longhand instead of using 1.
>
>>>   _CONST _PTR last = NULL;
>>>
>>>   while ((src = memchr (src, c, length)))
>>>     {
>>>       if (src > src_end)
>>> 	break;
>
>Also, src will never be > src_end - memchr returns NULL rather than
>reading beyond the bounds of length.
>
>>>       last = src;
>>>       src = (const unsigned char *) src + sizeof (unsigned char);
>>>     }
>>>
>>>   return (_PTR) last;
>>> }
>> 
>> The patch has a problem.  The loop misses to change the length variable
>> while iterating over the memchr results.  That means, any subsequent
>> call after the first one will potentiall read beyond the requested
>> length.  In border cases this will result in reading beyond the
>> available memory or beyond the allocated virtual memory.  The call to
>> memchr must never read beyond src + length - 1.
>
>Additionally, I think that searching forwards through the array via one
>function call per occurrence of the byte in question is wasteful - since
>we already know the array bounds, we might as well search in reverse by
>doing a single C loop that iterates backwards over a word at a time.
>strrchr must search forwards, because it is also searching for the
>terminating NUL and doesn't know the length in advance, but memrchr
>should be faster.

Why not just grab the source from FreeBSD?

http://svnweb.freebsd.org/base/head/lib/libc/string/memrchr.c?revision=178051&view=markup

(or actually from OpenBSD since that's where the above originated)

cgf