[Gluster-devel] glfs_readdir_r is painful

[Eric Blake]

On 10/30/2013 04:37 PM, Anand Avati wrote:
>> Thanks for starting that.  I see an off-by-one in that patch; pre-patch
>> you did:
>>
>> strncpy (dirent->d_name, gf_dirent->d_name, 256);
>>
>> but post-patch, you have:
>>
>> strncpy (dirent->d_name, gf_dirent->d_name, GF_NAME_MAX);
>>
>> with GF_NAME_MAX set to either NAME_MAX or 255.  This is a bug; you MUST
>> strncpy at least 1 byte more than the maximum name if you are to
>> guarantee a NUL-terminated d_name for the user.
>>
> 
> The buffer is guaranteed to be 0-inited,

Only if I call your new glfs_readdir; but if I call glfs_readdir_r, then
the buffer I pass in is NOT necessarily 0-inited.  If I call
glfs_readdir_r with a buffer smaller than d_name+padding out to 256, the
bug is mine (POSIX requires that readdir_r be used with at least
NAME_MAX+1 bytes).  But if I call glfs_readdir_r with a buffer of 256
bytes, and not 0-initialized, then a filename of 255 would have been
properly NUL-terminated with your pre-patch code, but NOT with your
post-patch code.

> and strncpy with 255 is now
> guaranteed to have a NULL terminated string no matter how big the name was
> (which wasn't the case before, in case the name was > 255 bytes).

No, remember that my argument is that XFS guarantees that you cannot
have a name > 255 (try it; XFS fails with ENAMETOOLONG starting at 256),
so we never had a problem with NUL-termination pre-patch (assuming
people guessed the correct NAME_MAX).  But the strncpy length parameter
has to be longer than the maximum length of the string being copied to
guarantee NUL termination.  You need to call strncpy(,,GF_NAME_MAX + 1)
because you cannot guarantee that the buffer I passed you has 0-inited
padding.

> 
> 
>>
>> Oh, and NAME_MAX is not guaranteed to be defined as 255; if it is larger
>> than 255 you are wasting memory compared to XFS, if it is less than 255
>> [although unlikely], you have made it impossible to return valid file
>> names to the user.  You may be better off just hard-coding GF_NAME_MAX
>> to 255 regardless of what the system has for its NAME_MAX.
>>
> 
> Hmm, I don't think so.. strncpy of 255 bytes on to a buffer guaranteed to
> be 256 or higher and also guaranteed to be 0-memset'ed cannot return an
> invalid file name. No?

The fact that your internal glfs_readdir buffer is memset means you are
safe there for a 255-byte filename; but that safety does not extend to
glfs_readdir_r for a user buffer.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://supercolony.gluster.org/pipermail/gluster-devel/attachments/20131030/c4682df2/attachment-0001.sig>