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
Attachment:
signature.asc
Description: OpenPGP digital signature
