Date: Wed, 10 Dec 2014 08:45:10 -0500 (EST) From: Rick Macklem <rmacklem@uoguelph.ca> To: Konstantin Belousov <kostikbel@gmail.com> Cc: FreeBSD Filesystems <freebsd-fs@freebsd.org>, George Neville-Neil <gnn@freebsd.org> Subject: Re: fuse dirent bug??? Message-ID: <1028205685.9208372.1418219110466.JavaMail.root@uoguelph.ca> In-Reply-To: <20141210092428.GE97072@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
Kostik wrote:
> On Tue, Dec 09, 2014 at 10:41:30PM -0500, Rick Macklem wrote:
> > Hi,
> >
> > While looking at the fuse code to change it to use a new
> > "struct dirent", I spotted this line, which doesn't look
> > correct.
> >
> > Line 358 of sys/fs/fuse/fuse_internal.c:
> > ((char *)cookediov->base)[bytesavail] = '\0';
> > - I think this is intended to null terminate the name,
> > since it comes right after the memcpy() of the file name.
> > However, bytesavail is the value returned by GENERIC_DIRSIZ(),
> > which means [bytesavail] after "cookediov->base" would be the
> > first byte after the "struct dirent" (including the space for
> > null termination and padding.
> >
> > If I'm correct, I think this line can be replaced by:
> > de->d_name[fudge->namelen] = '\0';
> > which would be the byte after the name in the structure.
> >
> > Also, although I think the first argument to the memcpy() call
> > just above this is correct, it is complex/convoluted.
> > Wouldn't just writing "memcpy(de->d_name, ..." make it
> > more readable?
> >
> > Anyone out there familiar with fuse able to look at/test this?
>
> No, I am not familiar with fuse. Still, I think you are right.
> OTOH, it is probably very rare to result in the actual override
> of the last byte after the buffer, since dirents have to fill the
> buffer to the last byte.
>
Ok, I took a closer look at the code and it seems that this bug
won't cause any problems.
1 - The calculation of the size of cookediov->base is bogus
(it uses fuse_dirent instead of dirent), but since fuse_dirent
is larger, the error makes the buffer too big.
--> writing '\0' one byte past the entry will never go past the
end of the buffer
This will need to be fixed when struct dirent becomes larger.
I'll include it in the patch I am working on.
2 - fiov_refresh() bzero()s the entire buffer and is called within
the loop, so the name will always be null terminated.
> One additional note. The getdirentries(2) specifies that the name
> must
> be null-terminated. But sys/dirent.h comment claims that the whole
> padding must be zeroed. I did not tracked the source of the buffer in
> fuse_internal_readdir(), so my question is whether the buffer is
> zeroed
> before filled. If not, padding must be cleared.
>
Well, I am aware of that comment and the NFS client has always done that.
However, from my recent glances at the code for other file system's
XX_readdir()s, most don't bother. (I think UFS, NFS and fuse are the
only ones that do 0 the pad bytes.)
Most (and I think ZFS is one of these) only put a single '\0' after the
name and I don't think they bzero() the buffer like fuse apparently does.
Because of the above, the copy_dirent32() function I am using doesn't
bother to 0 the pad bytes either and I haven't seen problems during
minimal testing. Zeroing the pad bytes could easily be added.
Btw, most file systems also ignore the "dirent shouldn't cross a 512 byte
block boundary either. (At one time I thought this was a requirement, since
UFS did it, but my guess is that userland code doesn't care.)
Most file systems (except UFS and NFS) just fill in "struct dirent"s packed
and return fewer bytes than requested when no more "struct dirent"s will
fit in the requested buffer size.
rick
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1028205685.9208372.1418219110466.JavaMail.root>