svn commit: r361995 - in head/lib/libc: gen tests/gen

[Kyle Evans]

Author: kevans
Date: Wed Jun 10 01:30:37 2020
New Revision: 361995
URL: https://svnweb.freebsd.org/changeset/base/361995

Log:
  execvp: fix up the ENOEXEC fallback
  
  If execve fails with ENOEXEC, execvp is expected to rebuild the command
  with /bin/sh instead and try again.
  
  The previous version did this, but overlooked two details:
  
  argv[0] can conceivably be NULL, in which case memp would never get
  terminated.  We must allocate no less than three * sizeof(char *) so we can
  properly terminate at all times. For the non-NULL argv standard case, we
  count all the non-NULL elements and actually skip the first argument, so we
  end up capturing the NULL terminator in our bcopy().
  
  The second detail is that the spec is actually worded such that we should
  have been preserving argv[0] as passed to execvp:
  
  "[...] executed command shall be as if the process invoked the sh utility
  using execl() as follows:
  
  execl(<shell path>, arg0, file, arg1, ..., (char *)0);
  
  where <shell path> is an unspecified pathname for the sh utility, file is
  the process image file, and for execvp(), where arg0, arg1, and so on
  correspond to the values passed to execvp() in argv[0], argv[1], and so on."
  
  So we make this change at this time as well, while we're already touching
  it. We decidedly can't preserve a NULL argv[0] as this would be incredibly,
  incredibly fragile, so we retain our legacy behavior of using "sh" for
  argv[] in this specific instance.
  
  Some light tests are added to try and detect some components of handling the
  ENOEXEC fallback; posix_spawnp_enoexec_fallback_null_argv0 is likely not
  100% reliable, but it at least won't raise false-alarms and it did result in
  useful failures with pre-change libc on my machine.
  
  This is a secondary change in D25038.
  
  Reported by:	Andrew Gierth <andrew_tao173.riddles.org.uk>
  Reviewed by:	jilles, kib, Andrew Gierth
  MFC after:	1 week

Modified:
  head/lib/libc/gen/exec.c
  head/lib/libc/tests/gen/Makefile
  head/lib/libc/tests/gen/posix_spawn_test.c

Modified: head/lib/libc/gen/exec.c
==============================================================================
--- head/lib/libc/gen/exec.c	Wed Jun 10 00:09:31 2020	(r361994)
+++ head/lib/libc/gen/exec.c	Wed Jun 10 01:30:37 2020	(r361995)
@@ -215,14 +215,28 @@ retry:		(void)_execve(bp, argv, envp);
 		case ENOEXEC:
 			for (cnt = 0; argv[cnt]; ++cnt)
 				;
-			memp = alloca((cnt + 2) * sizeof(char *));
+
+			/*
+			 * cnt may be 0 above; always allocate at least
+			 * 3 entries so that we can at least fit "sh", bp, and
+			 * the NULL terminator.  We can rely on cnt to take into
+			 * account the NULL terminator in all other scenarios,
+			 * as we drop argv[0].
+			 */
+			memp = alloca(MAX(3, cnt + 2) * sizeof(char *));
 			if (memp == NULL) {
 				/* errno = ENOMEM; XXX override ENOEXEC? */
 				goto done;
 			}
-			memp[0] = "sh";
-			memp[1] = bp;
-			bcopy(argv + 1, memp + 2, cnt * sizeof(char *));
+			if (cnt > 0) {
+				memp[0] = argv[0];
+				memp[1] = bp;
+				bcopy(argv + 1, memp + 2, cnt * sizeof(char *));
+			} else {
+				memp[0] = "sh";
+				memp[1] = bp;
+				memp[2] = NULL;
+			}
  			(void)_execve(_PATH_BSHELL,
 			    __DECONST(char **, memp), envp);
 			goto done;

Modified: head/lib/libc/tests/gen/Makefile
==============================================================================
--- head/lib/libc/tests/gen/Makefile	Wed Jun 10 00:09:31 2020	(r361994)
+++ head/lib/libc/tests/gen/Makefile	Wed Jun 10 01:30:37 2020	(r361995)
@@ -24,6 +24,15 @@ ATF_TESTS_C+=		wordexp_test
 # TODO: t_siginfo (fixes require further inspection)
 # TODO: t_sethostname_test (consistently screws up the hostname)
 
+FILESGROUPS+=		posix_spawn_test_FILES
+
+posix_spawn_test_FILES=	spawnp_enoexec.sh
+posix_spawn_test_FILESDIR=	${TESTSDIR}
+posix_spawn_test_FILESMODE= 0755
+posix_spawn_test_FILESOWN= root
+posix_spawn_test_FILESGRP= wheel
+posix_spawn_test_FILESPACKAGE=	${PACKAGE}
+
 CFLAGS+=	-DTEST_LONG_DOUBLE
 
 # Not sure why this isn't defined for all architectures, since most

Modified: head/lib/libc/tests/gen/posix_spawn_test.c
==============================================================================
--- head/lib/libc/tests/gen/posix_spawn_test.c	Wed Jun 10 00:09:31 2020	(r361994)
+++ head/lib/libc/tests/gen/posix_spawn_test.c	Wed Jun 10 01:30:37 2020	(r361995)
@@ -93,11 +93,50 @@ ATF_TC_BODY(posix_spawn_no_such_command_negative_test,
 	}
 }
 
+ATF_TC_WITHOUT_HEAD(posix_spawnp_enoexec_fallback);
+ATF_TC_BODY(posix_spawnp_enoexec_fallback, tc)
+{
+	char buf[FILENAME_MAX];
+	char *myargs[2];
+	int error, status;
+	pid_t pid, waitres;
+
+	snprintf(buf, sizeof(buf), "%s/spawnp_enoexec.sh",
+	    atf_tc_get_config_var(tc, "srcdir"));
+	myargs[0] = buf;
+	myargs[1] = NULL;
+	error = posix_spawnp(&pid, myargs[0], NULL, NULL, myargs, myenv);
+	ATF_REQUIRE(error == 0);
+	waitres = waitpid(pid, &status, 0);
+	ATF_REQUIRE(waitres == pid);
+	ATF_REQUIRE(WIFEXITED(status) && WEXITSTATUS(status) == 42);
+}
+
+ATF_TC_WITHOUT_HEAD(posix_spawnp_enoexec_fallback_null_argv0);
+ATF_TC_BODY(posix_spawnp_enoexec_fallback_null_argv0, tc)
+{
+	char buf[FILENAME_MAX];
+	char *myargs[1];
+	int error, status;
+	pid_t pid, waitres;
+
+	snprintf(buf, sizeof(buf), "%s/spawnp_enoexec.sh",
+	    atf_tc_get_config_var(tc, "srcdir"));
+	myargs[0] = NULL;
+	error = posix_spawnp(&pid, buf, NULL, NULL, myargs, myenv);
+	ATF_REQUIRE(error == 0);
+	waitres = waitpid(pid, &status, 0);
+	ATF_REQUIRE(waitres == pid);
+	ATF_REQUIRE(WIFEXITED(status) && WEXITSTATUS(status) == 42);
+}
+
 ATF_TP_ADD_TCS(tp)
 {
 
 	ATF_TP_ADD_TC(tp, posix_spawn_simple_test);
 	ATF_TP_ADD_TC(tp, posix_spawn_no_such_command_negative_test);
+	ATF_TP_ADD_TC(tp, posix_spawnp_enoexec_fallback);
+	ATF_TP_ADD_TC(tp, posix_spawnp_enoexec_fallback_null_argv0);
 
 	return (atf_no_error());
 }