Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 21 May 2017 17:25:35 +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:  <20170521142535.GI1622@kib.kiev.ua>
In-Reply-To: <20170521140355.GC21613@stack.nl>
References:  <20170420194314.GI1788@kib.kiev.ua> <20170521121456.GA21613@stack.nl> <20170521123118.GH1622@kib.kiev.ua> <20170521140355.GC21613@stack.nl>

next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, May 21, 2017 at 04:03:55PM +0200, Jilles Tjoelker wrote:
> On Sun, May 21, 2017 at 03:31:18PM +0300, Konstantin Belousov wrote:
> > 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 ?
> 
> Yes, my proposal is to change d_namlen to uint16_t.
> 
> Making it 32 bits is not useful with the 16-bit d_reclen, and increasing
> d_reclen does not seem useful to me with the current model of
> getdirentries() where the whole dirent must fit into the caller's
> buffer.
Bumping it now might cause less churn later, even if unused, but ok.

> 
> > I did not committed the change below, nor did I tested or even build it.
> 
> I'd like to skip overlong names in the native readdir_r() as well, so
> that long name support can be added to the kernel later without causing
> buffer overflows with applications using FreeBSD 12.0 libc.
> 
> The native readdir() does not seem to have such a problem.

Again, not even compiled.

diff --git a/lib/libc/gen/readdir-compat11.c b/lib/libc/gen/readdir-compat11.c
index 1c52f563c75..a865ab9157e 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 *
@@ -75,13 +79,15 @@ freebsd11_readdir(DIR *dirp)
 
 	if (__isthreaded)
 		_pthread_mutex_lock(&dirp->dd_lock);
-	dp = _readdir_unlocked(dirp, 1);
+	dp = _readdir_unlocked(dirp, RDU_SKIP);
 	if (dp != NULL) {
 		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 /* should not happen due to RDU_SHORT */
+			*result = NULL;
 	} else
 		*result = NULL;
 	return (0);
diff --git a/lib/libc/gen/readdir.c b/lib/libc/gen/readdir.c
index dc7b0df18b2..c30d48273b8 100644
--- a/lib/libc/gen/readdir.c
+++ b/lib/libc/gen/readdir.c
@@ -49,7 +49,7 @@ __FBSDID("$FreeBSD$");
  * get next entry in a directory.
  */
 struct dirent *
-_readdir_unlocked(DIR *dirp, int skip)
+_readdir_unlocked(DIR *dirp, int flags)
 {
 	struct dirent *dp;
 	long initial_seek;
@@ -80,10 +80,13 @@ _readdir_unlocked(DIR *dirp, int skip)
 		    dp->d_reclen > dirp->dd_len + 1 - dirp->dd_loc)
 			return (NULL);
 		dirp->dd_loc += dp->d_reclen;
-		if (dp->d_ino == 0 && skip)
+		if (dp->d_ino == 0 && (flags & RDU_SKIP) != 0)
 			continue;
 		if (dp->d_type == DT_WHT && (dirp->dd_flags & DTF_HIDEW))
 			continue;
+		if (dp->d_namlen >= sizeof(d_namlen) &&
+		    (flags & RDU_SHORT) != 0)
+			continue;
 		return (dp);
 	}
 }
@@ -91,15 +94,13 @@ _readdir_unlocked(DIR *dirp, int skip)
 struct dirent *
 readdir(DIR *dirp)
 {
-	struct dirent	*dp;
+	struct dirent *dp;
 
-	if (__isthreaded) {
+	if (__isthreaded)
 		_pthread_mutex_lock(&dirp->dd_lock);
-		dp = _readdir_unlocked(dirp, 1);
+	dp = _readdir_unlocked(dirp, RDU_SKIP);
+	if (__isthreaded)
 		_pthread_mutex_unlock(&dirp->dd_lock);
-	}
-	else
-		dp = _readdir_unlocked(dirp, 1);
 	return (dp);
 }
 
@@ -111,14 +112,13 @@ __readdir_r(DIR *dirp, struct dirent *entry, struct dirent **result)
 
 	saved_errno = errno;
 	errno = 0;
-	if (__isthreaded) {
+	if (__isthreaded)
 		_pthread_mutex_lock(&dirp->dd_lock);
-		if ((dp = _readdir_unlocked(dirp, 1)) != NULL)
-			memcpy(entry, dp, _GENERIC_DIRSIZ(dp));
-		_pthread_mutex_unlock(&dirp->dd_lock);
-	}
-	else if ((dp = _readdir_unlocked(dirp, 1)) != NULL)
+	dp = _readdir_unlocked(dirp, RDU_SKIP | RDU_SHORT);
+	if (dp != NULL)
 		memcpy(entry, dp, _GENERIC_DIRSIZ(dp));
+	if (__isthreaded)
+		_pthread_mutex_unlock(&dirp->dd_lock);
 
 	if (errno != 0) {
 		if (dp == NULL)
diff --git a/lib/libc/gen/telldir.h b/lib/libc/gen/telldir.h
index 96ff669a312..50adb351e91 100644
--- a/lib/libc/gen/telldir.h
+++ b/lib/libc/gen/telldir.h
@@ -66,4 +66,7 @@ void 		_reclaim_telldir(DIR *);
 void 		_seekdir(DIR *, long);
 void		_fixtelldir(DIR *dirp, long oldseek, long oldloc);
 
+#define	RDU_SKIP	0x0001
+#define	RDU_SHORT	0x0002
+
 #endif
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..472142f960b 100644
--- a/sys/sys/dirent.h
+++ b/sys/sys/dirent.h
@@ -58,8 +58,7 @@ typedef	__off_t		off_t;
  *
  * Explicit padding between the last member of the header (d_namelen) and
  * d_name avoids ABI padding at the end of dirent on LP64 architectures.
- * There is code depending on d_name being last.  Also, retaining this
- * padding on ILP32 architectures simplifies the compat32 layer.
+ * There is code depending on d_name being last.
  */
 
 struct dirent {
@@ -67,8 +66,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?20170521142535.GI1622>