[patch] rtld: fix fd leak with parallel dlopen and fork/exec

[Konstantin Belousov]

On Sun, Nov 04, 2012 at 03:37:27PM +0100, Jilles Tjoelker wrote:
> Rtld does not set FD_CLOEXEC on its internal file descriptors;
> therefore, such a file descriptor may be passed to a process created by
> another thread running in parallel to dlopen() or fdlopen().
> 
> The race is easy to trigger with the below dlopen_exec_race.c that
> performs the two in parallel repeatedly, for example
> ./dlopen_exec_race /lib/libedit.so.7 | grep lib
> 
> No other threads are expected to be running during parsing of the hints
> and libmap files but the file descriptors need not be passed to child
> processes so I added O_CLOEXEC there as well.
> 
> The O_CLOEXEC flag is ignored by older kernels so it will not cause
> breakage when people try the unsupported action of running new rtld on
> old kernel. However, the fcntl(F_DUPFD_CLOEXEC) will fail with [EINVAL]
> on an old kernel so this patch will break fdlopen() with old kernels. I
> suppose this is OK because fdlopen() is not used in the vital parts of
> buildworld and the like.
The patch should be fine. We definitely do not support running newer
binaries on older kernels, so the worries about the fdlopen(3) are
not sustained.

Please commit.

> 
> Index: libexec/rtld-elf/libmap.c
> ===================================================================
> --- libexec/rtld-elf/libmap.c	(revision 240561)
> +++ libexec/rtld-elf/libmap.c	(working copy)
> @@ -121,7 +121,7 @@
>  		}
>  	}
>  
> -	fd = open(rpath, O_RDONLY);
> +	fd = open(rpath, O_RDONLY | O_CLOEXEC);
>  	if (fd == -1) {
>  		dbg("lm_parse_file: open(\"%s\") failed, %s", rpath,
>  		    rtld_strerror(errno));
> Index: libexec/rtld-elf/rtld.c
> ===================================================================
> --- libexec/rtld-elf/rtld.c	(revision 240561)
> +++ libexec/rtld-elf/rtld.c	(working copy)
> @@ -1598,7 +1598,7 @@
>  		/* Keep from trying again in case the hints file is bad. */
>  		hints = "";
>  
> -		if ((fd = open(ld_elf_hints_path, O_RDONLY)) == -1)
> +		if ((fd = open(ld_elf_hints_path, O_RDONLY | O_CLOEXEC)) == -1)
>  			return (NULL);
>  		if (read(fd, &hdr, sizeof hdr) != sizeof hdr ||
>  		    hdr.magic != ELFHINTS_MAGIC ||
> @@ -2046,13 +2046,13 @@
>       */
>      fd = -1;
>      if (fd_u == -1) {
> -	if ((fd = open(path, O_RDONLY)) == -1) {
> +	if ((fd = open(path, O_RDONLY | O_CLOEXEC)) == -1) {
>  	    _rtld_error("Cannot open \"%s\"", path);
>  	    free(path);
>  	    return (NULL);
>  	}
>      } else {
> -	fd = dup(fd_u);
> +	fd = fcntl(fd_u, F_DUPFD_CLOEXEC, 0);
>  	if (fd == -1) {
>  	    _rtld_error("Cannot dup fd");
>  	    free(path);
> 
> 
> dlopen_exec_race.c:
> 
> #include <sys/types.h>
> #include <sys/wait.h>
> 
> #include <dlfcn.h>
> #include <err.h>
> #include <errno.h>
> #include <pthread.h>
> #include <spawn.h>
> #include <stdio.h>
> 
> extern char **environ;
> 
> static void *
> dlopen_thread(void *arg)
> {
> 	const char *path = arg;
> 	void *handle;
> 
> 	for (;;) {
> 		handle = dlopen(path, RTLD_LOCAL | RTLD_NOW);
> 		if (handle == NULL)
> 			continue;
> 		dlclose(handle);
> 	}
> 	return NULL;
> }
> 
> static void
> filestat_loop(void)
> {
> 	int error, status;
> 	pid_t pid, wpid;
> 
> 	for (;;) {
> 		error = posix_spawnp(&pid, "sh", NULL, NULL,
> 		    (char *[]){ "sh", "-c", "procstat -f $$", NULL },
> 		    environ);
> 		if (error != 0)
> 			errc(1, error, "posix_spawnp");
> 		do
> 			wpid = waitpid(pid, &status, 0);
> 		while (wpid == -1 && errno == EINTR);
> 		if (wpid == -1)
> 			err(1, "waitpid");
> 		if (status != 0)
> 			errx(1, "sh -c failed");
> 	}
> }
> 
> int
> main(int argc, char *argv[])
> {
> 	pthread_t td;
> 	int error;
> 
> 	if (argc != 2)
> 	{
> 		fprintf(stderr, "Usage: %s dso\n", argv[0]);
> 		return 1;
> 	}
> 
> 	error = pthread_create(&td, NULL, dlopen_thread, argv[1]);
> 	if (error != 0)
> 		errc(1, error, "pthread_create");
> 
> 	filestat_loop();
> 
> 	return 0;
> }
> 
> fdlopen_exec_race.c:
> 
> #include <sys/types.h>
> #include <sys/wait.h>
> 
> #include <dlfcn.h>
> #include <err.h>
> #include <errno.h>
> #include <fcntl.h>
> #include <pthread.h>
> #include <spawn.h>
> #include <stdio.h>
> #include <unistd.h>
> 
> extern char **environ;
> 
> static void *
> dlopen_thread(void *arg)
> {
> 	const char *path = arg;
> 	int fd;
> 	void *handle;
> 
> 	for (;;) {
> 		fd = open(path, O_RDONLY | O_CLOEXEC);
> 		if (fd == -1)
> 			err(1, "open %s", path);
> 		handle = fdlopen(fd, RTLD_LOCAL | RTLD_NOW);
> 		close(fd);
> 		if (handle == NULL)
> 			continue;
> 		dlclose(handle);
> 	}
> 	return NULL;
> }
> 
> static void
> filestat_loop(void)
> {
> 	int error, status;
> 	pid_t pid, wpid;
> 
> 	for (;;) {
> 		error = posix_spawnp(&pid, "sh", NULL, NULL,
> 		    (char *[]){ "sh", "-c", "procstat -f $$", NULL },
> 		    environ);
> 		if (error != 0)
> 			errc(1, error, "posix_spawnp");
> 		do
> 			wpid = waitpid(pid, &status, 0);
> 		while (wpid == -1 && errno == EINTR);
> 		if (wpid == -1)
> 			err(1, "waitpid");
> 		if (status != 0)
> 			errx(1, "sh -c failed");
> 	}
> }
> 
> int
> main(int argc, char *argv[])
> {
> 	pthread_t td;
> 	int error;
> 
> 	if (argc != 2)
> 	{
> 		fprintf(stderr, "Usage: %s dso\n", argv[0]);
> 		return 1;
> 	}
> 
> 	error = pthread_create(&td, NULL, dlopen_thread, argv[1]);
> 	if (error != 0)
> 		errc(1, error, "pthread_create");
> 
> 	filestat_loop();
> 
> 	return 0;
> }
> 
> -- 
> Jilles Tjoelker
> _______________________________________________
> freebsd-hackers at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "freebsd-hackers-unsubscribe at freebsd.org"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20121104/c43eaa92/attachment.sig>