[PATCH] sysopen -o cloexec doesn't work when the fd is dupped

[PATCH] sysopen -o cloexec doesn't work when the fd is dupped

[Stephane Chazelas]

[-- Attachment #1: Type: text/plain, Size: 1942 bytes --]

Hello:

On systems with  support for open(O_CLOEXEC):

$ zsh -c 'zmodload zsh/system; sysopen -w -o cloexec -u 3 /dev/null; ls -l /proc/self/fd'
total 0
lrwx------ 1 chazelas chazelas 64 May 23 16:35 0 -> /dev/pts/1
lrwx------ 1 chazelas chazelas 64 May 23 16:35 1 -> /dev/pts/1
lrwx------ 1 chazelas chazelas 64 May 23 16:35 2 -> /dev/pts/1
lr-x------ 1 chazelas chazelas 64 May 23 16:35 3 -> /proc/21668/fd

(OK)

$ zsh -c 'zmodload zsh/system; sysopen -w -o cloexec -u 4 /dev/null; ls -l /proc/self/fd'
total 0
lrwx------ 1 chazelas chazelas 64 May 23 16:35 0 -> /dev/pts/1
lrwx------ 1 chazelas chazelas 64 May 23 16:35 1 -> /dev/pts/1
lrwx------ 1 chazelas chazelas 64 May 23 16:35 2 -> /dev/pts/1
lr-x------ 1 chazelas chazelas 64 May 23 16:35 3 -> /proc/21669/fd
l-wx------ 1 chazelas chazelas 64 May 23 16:35 4 -> /dev/null

(not OK: fd 4 passed to ls)

$ zsh -c 'zmodload zsh/system; sysopen -w -o cloexec -u fd /dev/null; ls -l /proc/self/fd'
total 0
lrwx------ 1 chazelas chazelas 64 May 23 16:35 0 -> /dev/pts/1
lrwx------ 1 chazelas chazelas 64 May 23 16:35 1 -> /dev/pts/1
l-wx------ 1 chazelas chazelas 64 May 23 16:35 12 -> /dev/null
lrwx------ 1 chazelas chazelas 64 May 23 16:35 2 -> /dev/pts/1
lr-x------ 1 chazelas chazelas 64 May 23 16:35 3 -> /proc/21674/fd

(not OK: fd 3 passed to ls).

The reason is that in the last two cases, dup()/dup2() was
called on the fd returned by open(). In the first case to make
it the requested "3", and in the second case so it's a fd above
10.

The cloexec flag is one that is attached to the fd, not the open
file description, so it doesn't survive a dup(). So one needs to
reapply the CLOEXEC flag again after the dup.

See attached for a suggested fix. Linux has a dup3() which you
can pass the CLOEXEC flag to, but I don't expect that to  be
very portable.

The code could be simplified so that fcntl() is always called as
it's likely it's going to be called anyway.

-- 
Stephane

[-- Attachment #2: cloexec.fix --]
[-- Type: text/plain, Size: 1902 bytes --]

diff --git a/Src/Modules/system.c b/Src/Modules/system.c
index afaec262a..3eecd7e95 100644
--- a/Src/Modules/system.c
+++ b/Src/Modules/system.c
@@ -313,7 +313,7 @@ bin_sysopen(char *nam, char **args, Options ops, UNUSED(int func))
     int flags = O_NOCTTY | append | ((append || write) ?
 	(read ? O_RDWR : O_WRONLY) : O_RDONLY);
     char *opt, *ptr, *nextopt, *fdvar;
-    int o, fd, explicit = -1;
+    int o, fd, moved_fd, explicit = -1;
     mode_t perms = 0666;
 #if defined(FD_CLOEXEC) && !defined(O_CLOEXEC)
     int fdflags;
@@ -376,22 +376,32 @@ bin_sysopen(char *nam, char **args, Options ops, UNUSED(int func))
 	zwarnnam(nam, "can't open file %s: %e", *args, errno);
 	return 1;
     }
-    fd = (explicit > -1) ? redup(fd, explicit) : movefd(fd);
-    if (fd == -1) {
+    moved_fd = (explicit > -1) ? redup(fd, explicit) : movefd(fd);
+    if (moved_fd == -1) {
 	zwarnnam(nam, "can't open file %s", *args);
 	return 1;
     }
 
-#if defined(FD_CLOEXEC) && !defined(O_CLOEXEC)
+#ifdef FD_CLOEXEC
+#ifdef O_CLOEXEC
+    /*
+     * the O_CLOEXEC is a flag attached to the *file descriptor*, not the
+     * *open file description* so it doesn't survive a dup(). If that flag was
+     * requested and the fd was moved, we need to reapply it to the moved fd
+     * even if the original one was open with O_CLOEXEC
+     */
+    if ((flags & O_CLOEXEC) && fd != moved_fd)
+#else
     if (fdflags)
-	fcntl(fd, F_SETFD, FD_CLOEXEC);
-#endif
+#endif /* O_CLOEXEC */
+	fcntl(moved_fd, F_SETFD, FD_CLOEXEC);
+#endif /* FD_CLOEXEC */
     if (explicit == -1) {
-	fdtable[fd] = FDT_EXTERNAL;
-	setiparam(fdvar, fd);
-	/* if setting the variable failed, close fd to avoid leak */
+	fdtable[moved_fd] = FDT_EXTERNAL;
+	setiparam(fdvar, moved_fd);
+	/* if setting the variable failed, close moved_fd to avoid leak */
 	if (errflag)
-	    zclose(fd);
+	    zclose(moved_fd);
     }
 
     return 0;