Re: [PATCH] add memrchr(3)

[Eric Blake <eblake at redhat dot com>]

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.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature