Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 4 Nov 2012 16:49:24 +0200
From:      Konstantin Belousov <kostikbel@gmail.com>
To:        Jilles Tjoelker <jilles@stack.nl>
Cc:        freebsd-hackers@freebsd.org
Subject:   Re: [patch] rtld: fix fd leak with parallel dlopen and fork/exec
Message-ID:  <20121104144924.GN73505@kib.kiev.ua>
In-Reply-To: <20121104143727.GB35520@stack.nl>
References:  <20121104143727.GB35520@stack.nl>

next in thread | previous in thread | raw e-mail | index | archive | help

--p0HNO2YbtFeVXwJ3
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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().
>=20
> 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
>=20
> 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.
>=20
> 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.

>=20
> Index: libexec/rtld-elf/libmap.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- libexec/rtld-elf/libmap.c	(revision 240561)
> +++ libexec/rtld-elf/libmap.c	(working copy)
> @@ -121,7 +121,7 @@
>  		}
>  	}
> =20
> -	fd =3D open(rpath, O_RDONLY);
> +	fd =3D open(rpath, O_RDONLY | O_CLOEXEC);
>  	if (fd =3D=3D -1) {
>  		dbg("lm_parse_file: open(\"%s\") failed, %s", rpath,
>  		    rtld_strerror(errno));
> Index: libexec/rtld-elf/rtld.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- 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 =3D "";
> =20
> -		if ((fd =3D open(ld_elf_hints_path, O_RDONLY)) =3D=3D -1)
> +		if ((fd =3D open(ld_elf_hints_path, O_RDONLY | O_CLOEXEC)) =3D=3D -1)
>  			return (NULL);
>  		if (read(fd, &hdr, sizeof hdr) !=3D sizeof hdr ||
>  		    hdr.magic !=3D ELFHINTS_MAGIC ||
> @@ -2046,13 +2046,13 @@
>       */
>      fd =3D -1;
>      if (fd_u =3D=3D -1) {
> -	if ((fd =3D open(path, O_RDONLY)) =3D=3D -1) {
> +	if ((fd =3D open(path, O_RDONLY | O_CLOEXEC)) =3D=3D -1) {
>  	    _rtld_error("Cannot open \"%s\"", path);
>  	    free(path);
>  	    return (NULL);
>  	}
>      } else {
> -	fd =3D dup(fd_u);
> +	fd =3D fcntl(fd_u, F_DUPFD_CLOEXEC, 0);
>  	if (fd =3D=3D -1) {
>  	    _rtld_error("Cannot dup fd");
>  	    free(path);
>=20
>=20
> dlopen_exec_race.c:
>=20
> #include <sys/types.h>
> #include <sys/wait.h>
>=20
> #include <dlfcn.h>
> #include <err.h>
> #include <errno.h>
> #include <pthread.h>
> #include <spawn.h>
> #include <stdio.h>
>=20
> extern char **environ;
>=20
> static void *
> dlopen_thread(void *arg)
> {
> 	const char *path =3D arg;
> 	void *handle;
>=20
> 	for (;;) {
> 		handle =3D dlopen(path, RTLD_LOCAL | RTLD_NOW);
> 		if (handle =3D=3D NULL)
> 			continue;
> 		dlclose(handle);
> 	}
> 	return NULL;
> }
>=20
> static void
> filestat_loop(void)
> {
> 	int error, status;
> 	pid_t pid, wpid;
>=20
> 	for (;;) {
> 		error =3D posix_spawnp(&pid, "sh", NULL, NULL,
> 		    (char *[]){ "sh", "-c", "procstat -f $$", NULL },
> 		    environ);
> 		if (error !=3D 0)
> 			errc(1, error, "posix_spawnp");
> 		do
> 			wpid =3D waitpid(pid, &status, 0);
> 		while (wpid =3D=3D -1 && errno =3D=3D EINTR);
> 		if (wpid =3D=3D -1)
> 			err(1, "waitpid");
> 		if (status !=3D 0)
> 			errx(1, "sh -c failed");
> 	}
> }
>=20
> int
> main(int argc, char *argv[])
> {
> 	pthread_t td;
> 	int error;
>=20
> 	if (argc !=3D 2)
> 	{
> 		fprintf(stderr, "Usage: %s dso\n", argv[0]);
> 		return 1;
> 	}
>=20
> 	error =3D pthread_create(&td, NULL, dlopen_thread, argv[1]);
> 	if (error !=3D 0)
> 		errc(1, error, "pthread_create");
>=20
> 	filestat_loop();
>=20
> 	return 0;
> }
>=20
> fdlopen_exec_race.c:
>=20
> #include <sys/types.h>
> #include <sys/wait.h>
>=20
> #include <dlfcn.h>
> #include <err.h>
> #include <errno.h>
> #include <fcntl.h>
> #include <pthread.h>
> #include <spawn.h>
> #include <stdio.h>
> #include <unistd.h>
>=20
> extern char **environ;
>=20
> static void *
> dlopen_thread(void *arg)
> {
> 	const char *path =3D arg;
> 	int fd;
> 	void *handle;
>=20
> 	for (;;) {
> 		fd =3D open(path, O_RDONLY | O_CLOEXEC);
> 		if (fd =3D=3D -1)
> 			err(1, "open %s", path);
> 		handle =3D fdlopen(fd, RTLD_LOCAL | RTLD_NOW);
> 		close(fd);
> 		if (handle =3D=3D NULL)
> 			continue;
> 		dlclose(handle);
> 	}
> 	return NULL;
> }
>=20
> static void
> filestat_loop(void)
> {
> 	int error, status;
> 	pid_t pid, wpid;
>=20
> 	for (;;) {
> 		error =3D posix_spawnp(&pid, "sh", NULL, NULL,
> 		    (char *[]){ "sh", "-c", "procstat -f $$", NULL },
> 		    environ);
> 		if (error !=3D 0)
> 			errc(1, error, "posix_spawnp");
> 		do
> 			wpid =3D waitpid(pid, &status, 0);
> 		while (wpid =3D=3D -1 && errno =3D=3D EINTR);
> 		if (wpid =3D=3D -1)
> 			err(1, "waitpid");
> 		if (status !=3D 0)
> 			errx(1, "sh -c failed");
> 	}
> }
>=20
> int
> main(int argc, char *argv[])
> {
> 	pthread_t td;
> 	int error;
>=20
> 	if (argc !=3D 2)
> 	{
> 		fprintf(stderr, "Usage: %s dso\n", argv[0]);
> 		return 1;
> 	}
>=20
> 	error =3D pthread_create(&td, NULL, dlopen_thread, argv[1]);
> 	if (error !=3D 0)
> 		errc(1, error, "pthread_create");
>=20
> 	filestat_loop();
>=20
> 	return 0;
> }
>=20
> --=20
> Jilles Tjoelker
> _______________________________________________
> freebsd-hackers@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"

--p0HNO2YbtFeVXwJ3
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)

iEYEARECAAYFAlCWgHMACgkQC3+MBN1Mb4g24QCfVN8037vLPSuN26+SSURSo0jF
7usAoJQMVfGhVmD1qkO/J29bHMVGpBhv
=to7n
-----END PGP SIGNATURE-----

--p0HNO2YbtFeVXwJ3--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121104144924.GN73505>