[File] [PATCH v2] multithreaded fix: open file and create pipes with O_CLOEXEC

[Denys Vlasenko]

(v2: SET_FD -> F_SETFD)

rpmbuild recently implemented multithreaded analysis of files
which are going into resultant .rpm

This uncovered a bug: if two parallel magic_file() calls
analyze two large xz-compressed files, both spawn "xz -cd"
uncompressing child, which writes out uncompressed data.
We read some portion, then close the pipe, then waitpid() the child.
If uncompressed data is larger, child shound get EPIPE and exit.

However, with two *parallel* calls OTHER child may unintentially inherit
pipe fds, thus keeping pipe open and making writes in *our* child
block instead of failing with EPIPE!

For bug to occur, two threads must mutually inherit their pipes,
and both must have large outputs:

= If only one child inherits other child's pipe fds,
it eventually completes, which implicitly closes all fds
including inherited pipe fds, which will make
other child's write() finally fails with EPIPE,
and the bug goes unnoticed.

= If either child has output less than 1 megabyte, then written data
is fully consumed (write() does not block) and the child exits,
and the bug goes unnoticed.

Signed-off-by: Denys Vlasenko <dvlasenk at redhat.com>
CC: dzickus at redhat.com

diff -urpN file-5.39.ORIG/src/compress.c file-5.39/src/compress.c
--- file-5.39.ORIG/src/compress.c	2020-05-31 02:11:06.000000000 +0200
+++ file-5.39/src/compress.c	2020-12-08 11:39:30.106867334 +0100
@@ -76,6 +76,15 @@ typedef void (*sig_t)(int);
 #include <lzma.h>
 #endif
 
+/* open(O_CLOEXEC) and pipe2() are Linuxisms */
+#ifndef O_CLOEXEC
+# define O_CLOEXEC 0
+# define pipe2(fds,flags) pipe(fds)
+# define clear_O_CLOEXEC(fd) ((void)0)
+#else
+# define clear_O_CLOEXEC(fd) fcntl(fd, F_SETFD, 0)
+#endif
+
 #ifdef DEBUG
 int tty = -1;
 #define DPRINTF(...)	do { \
@@ -844,8 +853,22 @@ uncompressbuf(int fd, size_t bytes_max,
 	for (i = 0; i < __arraycount(fdp); i++)
 		fdp[i][0] = fdp[i][1] = -1;
 
-	if ((fd == -1 && pipe(fdp[STDIN_FILENO]) == -1) ||
-	    pipe(fdp[STDOUT_FILENO]) == -1 || pipe(fdp[STDERR_FILENO]) == -1) {
+	/* There are multithreaded users who run magic_file()
+	 * from dozens of threads. If two parallel magic_file() calls
+	 * analyze two large compressed files, both will spawn
+	 * an uncompressing child here, which writes out uncompressed data.
+	 * We read some portion, then close the pipe, then waitpid() the child.
+	 * If uncompressed data is larger, child shound get EPIPE and exit.
+	 * However, with *parallel* calls OTHER child may unintentially inherit
+	 * pipe fds, thus keeping pipe open and making writes in our child
+	 * block instead of failing with EPIPE!
+	 * (For bug to occur, two threads must mutually inherit their pipes,
+	 * and both must have large outputs. Thus it happens not that often).
+	 * To avoid this, be sure to create pipes with O_CLOEXEC.
+	 */
+	if ((fd == -1 && pipe2(fdp[STDIN_FILENO], O_CLOEXEC) == -1) ||
+	    pipe2(fdp[STDOUT_FILENO], O_CLOEXEC) == -1 ||
+	    pipe2(fdp[STDERR_FILENO], O_CLOEXEC) == -1) {
 		closep(fdp[STDIN_FILENO]);
 		closep(fdp[STDOUT_FILENO]);
 		return makeerror(newch, n, "Cannot create pipe, %s",
@@ -876,16 +899,20 @@ uncompressbuf(int fd, size_t bytes_max,
 			if (fdp[STDIN_FILENO][1] > 2)
 				(void) close(fdp[STDIN_FILENO][1]);
 		}
+		clear_O_CLOEXEC(STDIN_FILENO);
+
 ///FIXME: if one of the fdp[i][j] is 0 or 1, this can bomb spectacularly
 		if (copydesc(STDOUT_FILENO, fdp[STDOUT_FILENO][1]))
 			(void) close(fdp[STDOUT_FILENO][1]);
 		if (fdp[STDOUT_FILENO][0] > 2)
 			(void) close(fdp[STDOUT_FILENO][0]);
+		clear_O_CLOEXEC(STDOUT_FILENO);
 
 		if (copydesc(STDERR_FILENO, fdp[STDERR_FILENO][1]))
 			(void) close(fdp[STDERR_FILENO][1]);
 		if (fdp[STDERR_FILENO][0] > 2)
 			(void) close(fdp[STDERR_FILENO][0]);
+		clear_O_CLOEXEC(STDERR_FILENO);
 
 		(void)execvp(compr[method].argv[0],
 		    RCAST(char *const *, RCAST(intptr_t, compr[method].argv)));
diff -urpN file-5.39.ORIG/src/magic.c file-5.39/src/magic.c
--- file-5.39.ORIG/src/magic.c	2020-06-15 02:01:01.000000000 +0200
+++ file-5.39/src/magic.c	2020-12-08 11:39:52.938808880 +0100
@@ -69,6 +69,11 @@ FILE_RCSID("@(#)$File: magic.c,v 1.112 2
 #endif
 #endif
 
+/* open(O_CLOEXEC) and pipe2() are Linuxisms */
+#ifndef O_CLOEXEC
+# define O_CLOEXEC 0
+#endif
+
 private void close_and_restore(const struct magic_set *, const char *, int,
     const struct stat *);
 private int unreadable_info(struct magic_set *, mode_t, const char *);
@@ -436,7 +441,7 @@ file_or_fd(struct magic_set *ms, const c
 		_setmode(STDIN_FILENO, O_BINARY);
 #endif
 	if (inname != NULL) {
-		int flags = O_RDONLY|O_BINARY|O_NONBLOCK;
+		int flags = O_RDONLY|O_BINARY|O_NONBLOCK|O_CLOEXEC;
 		errno = 0;
 		if ((fd = open(inname, flags)) < 0) {
 			okstat = stat(inname, &sb) == 0;