Date: Sun, 21 May 2017 15:31:18 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Jilles Tjoelker <jilles@stack.nl> Cc: freebsd-current@freebsd.org, freebsd-fs@freebsd.org, freebsd-ports@freebsd.org, emaste@freebsd.org, Kirk McKusick <mckusick@mckusick.com> Subject: Re: 64-bit inodes (ino64) Status Update and Call for Testing Message-ID: <20170521123118.GH1622@kib.kiev.ua> In-Reply-To: <20170521121456.GA21613@stack.nl> References: <20170420194314.GI1788@kib.kiev.ua> <20170521121456.GA21613@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, May 21, 2017 at 02:14:56PM +0200, Jilles Tjoelker wrote:
> We have another type in this area which is too small in some situations:
> uint8_t for struct dirent.d_namlen. For filesystems that store filenames
> as upto 255 UTF-16 code units, the name to be stored in d_name may be
> upto 765 bytes long in UTF-8. This was reported in PR 204643. The code
> currently handles this by returning the short (8.3) name, but this name
> may not be present or usable, leaving the file inaccessible.
>
> Actually allowing longer names seems too complicated to add to the ino64
> change, but changing d_namlen to uint16_t (using d_pad0 space) and
> skipping entries with d_namlen > 255 in libc may be helpful.
>
> Note that applications using the deprecated readdir_r() will not be able
> to read such long names, since the API does not allow specifying that a
> larger buffer has been provided. (This could be avoided by making struct
> dirent.d_name 766 bytes long instead of 256.)
>
> Unfortunately, the existence of readdir_r() also prevents changing
> struct dirent.d_name to the more correct flexible array.
Yes, changing the size of d_name at this stage of the project is out of
question. My reading of your proposal is that we should extend the size
of d_namlen to uint16_t, am I right ? Should we go to 32bit directly
then, perhaps ?
I did not committed the change below, nor did I tested or even build it.
diff --git a/lib/libc/gen/readdir-compat11.c b/lib/libc/gen/readdir-compat11.c
index 1c52f563c75..18d85adaa63 100644
--- a/lib/libc/gen/readdir-compat11.c
+++ b/lib/libc/gen/readdir-compat11.c
@@ -41,6 +41,7 @@ __FBSDID("$FreeBSD$");
#define _WANT_FREEBSD11_DIRENT
#include <dirent.h>
#include <errno.h>
+#include <stdbool.h>
#include <stddef.h>
#include <stdlib.h>
#include <string.h>
@@ -53,10 +54,12 @@ __FBSDID("$FreeBSD$");
#include "gen-compat.h"
-static void
+static bool
freebsd11_cvtdirent(struct freebsd11_dirent *dstdp, struct dirent *srcdp)
{
+ if (srcdp->d_namelen >= sizeof(dstdp->d_name))
+ return (false);
dstdp->d_type = srcdp->d_type;
dstdp->d_namlen = srcdp->d_namlen;
dstdp->d_fileno = srcdp->d_fileno; /* truncate */
@@ -65,6 +68,7 @@ freebsd11_cvtdirent(struct freebsd11_dirent *dstdp, struct dirent *srcdp)
bzero(dstdp->d_name + dstdp->d_namlen,
dstdp->d_reclen - offsetof(struct freebsd11_dirent, d_name) -
dstdp->d_namlen);
+ return (true);
}
struct freebsd11_dirent *
@@ -80,8 +84,10 @@ freebsd11_readdir(DIR *dirp)
if (dirp->dd_compat_de == NULL)
dirp->dd_compat_de = malloc(sizeof(struct
freebsd11_dirent));
- freebsd11_cvtdirent(dirp->dd_compat_de, dp);
- dstdp = dirp->dd_compat_de;
+ if (freebsd11_cvtdirent(dirp->dd_compat_de, dp))
+ dstdp = dirp->dd_compat_de;
+ else
+ dstdp = NULL;
} else
dstdp = NULL;
if (__isthreaded)
@@ -101,8 +107,10 @@ freebsd11_readdir_r(DIR *dirp, struct freebsd11_dirent *entry,
if (error != 0)
return (error);
if (xresult != NULL) {
- freebsd11_cvtdirent(entry, &xentry);
- *result = entry;
+ if (freebsd11_cvtdirent(entry, &xentry))
+ *result = entry;
+ else
+ *result = NULL;
} else
*result = NULL;
return (0);
diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c
index 784af836aee..27b2635030d 100644
--- a/sys/kern/vfs_syscalls.c
+++ b/sys/kern/vfs_syscalls.c
@@ -3733,7 +3733,8 @@ freebsd11_kern_getdirentries(struct thread *td, int fd, char *ubuf, u_int count,
if (dp->d_reclen == 0)
break;
MPASS(dp->d_reclen >= _GENERIC_DIRLEN(0));
- /* dp->d_namlen <= sizeof(dstdp.d_name) - 1 always */
+ if (dp->d_namlen >= sizeof(dstdp.d_name))
+ continue;
dstdp.d_type = dp->d_type;
dstdp.d_namlen = dp->d_namlen;
dstdp.d_fileno = dp->d_fileno; /* truncate */
diff --git a/sys/sys/dirent.h b/sys/sys/dirent.h
index 341855d0530..691c4e8f90f 100644
--- a/sys/sys/dirent.h
+++ b/sys/sys/dirent.h
@@ -67,8 +67,9 @@ struct dirent {
off_t d_off; /* directory offset of entry */
__uint16_t d_reclen; /* length of this record */
__uint8_t d_type; /* file type, see below */
- __uint8_t d_namlen; /* length of string in d_name */
- __uint32_t d_pad0;
+ __uint8_t d_pad0
+ __uint16_t d_namlen; /* length of string in d_name */
+ __uint16_t d_pad1;
#if __BSD_VISIBLE
#define MAXNAMLEN 255
char d_name[MAXNAMLEN + 1]; /* name must be no longer than this */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170521123118.GH1622>