Skip to content

gh-131064: Handle closed pipes consistently in communicate #159855

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
limadog9 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

gh-131064: Handle closed pipes consistently in communicate #159855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions Lib/subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1368,6 +1368,8 @@ def communicate(self, input=None, timeout=None):

if self._communication_started and input:
raise ValueError("Cannot send input after starting communication")
if input and self.stdin is not None and self.stdin.closed:
raise ValueError("Cannot send input to closed stdin")

# Optimization: If we are not worried about timeouts, we haven't
# started communicating, and we have one or zero pipes, using select()
Expand All @@ -1376,13 +1378,14 @@ def communicate(self, input=None, timeout=None):
[self.stdin, self.stdout, self.stderr].count(None) >= 2):
stdout = None
stderr = None
empty_output = '' if self.text_mode else b''
if self.stdin:
self._stdin_write(input)
elif self.stdout:
stdout = self.stdout.read()
stdout = self.stdout.read() if not self.stdout.closed else empty_output
self.stdout.close()
elif self.stderr:
stderr = self.stderr.read()
stderr = self.stderr.read() if not self.stderr.closed else empty_output
self.stderr.close()
self.wait()
else:
Expand Down Expand Up @@ -1801,8 +1804,9 @@ def _wait(self, timeout):


def _readerthread(self, fh, buffer):
buffer.append(fh.read())
fh.close()
if not fh.closed:
buffer.append(fh.read())
fh.close()


def _writerthread(self, input):
Expand Down Expand Up @@ -1869,8 +1873,9 @@ def _communicate(self, input, endtime, orig_timeout):
self.stderr.close()

# All data exchanged. Translate lists into strings.
stdout = stdout[0] if stdout else None
stderr = stderr[0] if stderr else None
empty_output = '' if self.text_mode else b''
stdout = empty_output.join(stdout) if stdout is not None else None
stderr = empty_output.join(stderr) if stderr is not None else None

@pekkaklarck pekkaklarck Oct 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that these changes aren't needed. At this point stdout and stderr are either None or lists self._stdout_buff or self._stderr_buff, respectively. These lists earlier contained exactly one value, but now they can also be empty. The existing if stdout/if stderr conditions cover both the None case and then empty list case.

I consider the current code rather confusing, though, and would consider changing it to something like this:

stdout = None
stderr = None
if self.stdout:
    self.stdout.close()
    if self._stdout_buff:
        stdout = self._stdout_buff[0]
if self.stderr:
    self.stderr.close()
    if self._stderr_buff:
        stderr = self._stderr_buff[0]

return (stdout, stderr)


return (stdout, stderr)

Expand Down
41 changes: 39 additions & 2 deletions Lib/test/test_subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,6 +943,43 @@ def test_communicate_stderr(self):
self.assertEqual(stdout, None)
self.assertEqual(stderr, b"pineapple")

def test_communicate_closed_output_pipe(self):
for stream, extra_pipe, text, timeout in itertools.product(
('stdout', 'stderr'), (False, True), (False, True),
(None, support.SHORT_TIMEOUT),
):
with self.subTest(stream=stream, extra_pipe=extra_pipe,
text=text, timeout=timeout):
pipes = {stream: subprocess.PIPE}
if extra_pipe:
pipes.update(stdout=subprocess.PIPE, stderr=subprocess.PIPE)
with subprocess.Popen(ZERO_RETURN_CMD, text=text, **pipes) as p:
getattr(p, stream).close()
output = p.communicate(timeout=timeout)
empty = '' if text else b''
self.assertEqual(output, tuple(
empty if name in pipes else None
for name in ('stdout', 'stderr')))
self.assertEqual(p.returncode, 0)

def test_communicate_closed_stdin_with_input(self):
for extra_pipe, text, timeout in itertools.product(
(False, True), (False, True), (None, support.SHORT_TIMEOUT),
):
with self.subTest(extra_pipe=extra_pipe, text=text, timeout=timeout):
with subprocess.Popen(
ZERO_RETURN_CMD, stdin=subprocess.PIPE, text=text,
stdout=subprocess.PIPE if extra_pipe else None,
) as p:
p.stdin.close()
with self.assertRaisesRegex(ValueError, 'closed stdin'):
p.communicate('unused' if text else b'unused',
timeout=timeout)
output = p.communicate(timeout=timeout)
empty = '' if text else b''
self.assertEqual(output, (empty if extra_pipe else None, None))
self.assertEqual(p.returncode, 0)

def test_communicate(self):
p = subprocess.Popen([sys.executable, "-c",
'import sys,os;'
Expand Down Expand Up @@ -3557,7 +3594,7 @@ def test_communicate_BrokenPipeError_stdin_write(self):
# By not setting stdout or stderr or a timeout we force the fast path
# that just calls _stdin_write() internally due to our mock.
proc = subprocess.Popen(ZERO_RETURN_CMD)
with proc, mock.patch.object(proc, 'stdin') as mock_proc_stdin:
with proc, mock.patch.object(proc, 'stdin', closed=False) as mock_proc_stdin:
mock_proc_stdin.write.side_effect = BrokenPipeError
proc.communicate(b'stuff') # Should swallow the BrokenPipeError.
mock_proc_stdin.write.assert_called_once_with(b'stuff')
Expand All @@ -3569,7 +3606,7 @@ def test_communicate_BrokenPipeError_stdin_flush(self):
proc = subprocess.Popen([sys.executable, '-h'],
stdin=subprocess.PIPE,
stdout=subprocess.PIPE)
with proc, mock.patch.object(proc, 'stdin') as mock_proc_stdin, \
with proc, mock.patch.object(proc, 'stdin', closed=False) as mock_proc_stdin, \
open(os.devnull, 'wb') as dev_null:
mock_proc_stdin.flush.side_effect = BrokenPipeError
# because _communicate registers a selector using proc.stdin...
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2026-10-20-04-15-00.gh-issue-131064.n4Bw8T.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Make :meth:`subprocess.Popen.communicate` consistently handle pipes closed
before the call. Closed output pipes produce empty output. Sending nonempty
input to a closed input pipe raises :exc:`ValueError`.
Loading