diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 93262df7ab4..83c79ef6245 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -1169,6 +1169,7 @@ class Popen(object): if executable is None: executable = args[0] + orig_executable = executable # For transferring possible exec failure from child to parent. # Data format: "exception name:hex errno:description" @@ -1224,6 +1225,7 @@ class Popen(object): self._child_created = True if self.pid == 0: # Child + reached_preexec = False try: # Close parent's pipe ends if p2cwrite != -1: @@ -1288,6 +1290,7 @@ class Popen(object): if start_new_session and hasattr(os, 'setsid'): os.setsid() + reached_preexec = True if preexec_fn: preexec_fn() @@ -1303,6 +1306,8 @@ class Popen(object): errno_num = exc_value.errno else: errno_num = 0 + if not reached_preexec: + exc_value = "noexec" message = '%s:%x:%s' % (exc_type.__name__, errno_num, exc_value) message = message.encode(errors="surrogatepass") @@ -1364,10 +1369,17 @@ class Popen(object): err_msg = err_msg.decode(errors="surrogatepass") if issubclass(child_exception_type, OSError) and hex_errno: errno_num = int(hex_errno, 16) + child_exec_never_called = (err_msg == "noexec") + if child_exec_never_called: + err_msg = "" if errno_num != 0: err_msg = os.strerror(errno_num) if errno_num == errno.ENOENT: - err_msg += ': ' + repr(args[0]) + if child_exec_never_called: + # The error must be from chdir(cwd). + err_msg += ': ' + repr(cwd) + else: + err_msg += ': ' + repr(orig_executable) raise child_exception_type(errno_num, err_msg) raise child_exception_type(err_msg) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 28796411106..15fb498d246 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -884,24 +884,30 @@ class _SuppressCoreFiles(object): @unittest.skipIf(mswindows, "POSIX specific tests") class POSIXProcessTestCase(BaseTestCase): - def test_exceptions(self): - nonexistent_dir = "/_this/pa.th/does/not/exist" + def setUp(self): + super().setUp() + self._nonexistent_dir = "/_this/pa.th/does/not/exist" + + def _get_chdir_exception(self): try: - os.chdir(nonexistent_dir) + os.chdir(self._nonexistent_dir) except OSError as e: # This avoids hard coding the errno value or the OS perror() # string and instead capture the exception that we want to see # below for comparison. desired_exception = e - desired_exception.strerror += ': ' + repr(sys.executable) + desired_exception.strerror += ': ' + repr(self._nonexistent_dir) else: self.fail("chdir to nonexistant directory %s succeeded." % - nonexistent_dir) + self._nonexistent_dir) + return desired_exception - # Error in the child re-raised in the parent. + def test_exception_cwd(self): + """Test error in the child raised in the parent for a bad cwd.""" + desired_exception = self._get_chdir_exception() try: p = subprocess.Popen([sys.executable, "-c", ""], - cwd=nonexistent_dir) + cwd=self._nonexistent_dir) except OSError as e: # Test that the child process chdir failure actually makes # it up to the parent process as the correct exception. @@ -910,6 +916,33 @@ class POSIXProcessTestCase(BaseTestCase): else: self.fail("Expected OSError: %s" % desired_exception) + def test_exception_bad_executable(self): + """Test error in the child raised in the parent for a bad executable.""" + desired_exception = self._get_chdir_exception() + try: + p = subprocess.Popen([sys.executable, "-c", ""], + executable=self._nonexistent_dir) + except OSError as e: + # Test that the child process exec failure actually makes + # it up to the parent process as the correct exception. + self.assertEqual(desired_exception.errno, e.errno) + self.assertEqual(desired_exception.strerror, e.strerror) + else: + self.fail("Expected OSError: %s" % desired_exception) + + def test_exception_bad_args_0(self): + """Test error in the child raised in the parent for a bad args[0].""" + desired_exception = self._get_chdir_exception() + try: + p = subprocess.Popen([self._nonexistent_dir, "-c", ""]) + except OSError as e: + # Test that the child process exec failure actually makes + # it up to the parent process as the correct exception. + self.assertEqual(desired_exception.errno, e.errno) + self.assertEqual(desired_exception.strerror, e.strerror) + else: + self.fail("Expected OSError: %s" % desired_exception) + def test_restore_signals(self): # Code coverage for both values of restore_signals to make sure it # at least does not blow up. diff --git a/Misc/NEWS b/Misc/NEWS index 759a12cffd1..9e1e96b55e4 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -129,6 +129,10 @@ Core and Builtins Library ------- +- Issue #16114: The subprocess module no longer provides a misleading + error message stating that args[0] did not exist when either the cwd or + executable keyword arguments specified a path that did not exist. + - Issue #15756: subprocess.poll() now properly handles errno.ECHILD to return a returncode of 0 when the child has already exited or cannot be waited on. diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 59673f4969f..19ca31f56dd 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -354,7 +354,7 @@ child_exec(char *const exec_array[], PyObject *preexec_fn, PyObject *preexec_fn_args_tuple) { - int i, saved_errno, unused; + int i, saved_errno, unused, reached_preexec = 0; PyObject *result; const char* err_msg = ""; /* Buffer large enough to hold a hex integer. We can't malloc. */ @@ -438,6 +438,7 @@ child_exec(char *const exec_array[], POSIX_CALL(setsid()); #endif + reached_preexec = 1; if (preexec_fn != Py_None && preexec_fn_args_tuple) { /* This is where the user has asked us to deadlock their program. */ result = PyObject_Call(preexec_fn, preexec_fn_args_tuple, NULL); @@ -487,6 +488,10 @@ error: } unused = write(errpipe_write, cur, hex_errno + sizeof(hex_errno) - cur); unused = write(errpipe_write, ":", 1); + if (!reached_preexec) { + /* Indicate to the parent that the error happened before exec(). */ + unused = write(errpipe_write, "noexec", 6); + } /* We can't call strerror(saved_errno). It is not async signal safe. * The parent process will look the error message up. */ } else {