Skip to content

gh-153970: Validate the returncode type in CalledProcessError #153971

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
fedonman wants to merge 10 commits into
base: main
Choose a base branch
from
Open

gh-153970: Validate the returncode type in CalledProcessError #153971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e4ebd4f
gh-153970: Fix CalledProcessError.__str__ crash when returncode is None
fedonman Jul 18, 2026
7162066
change implementation per comment by vstinner
fedonman Jul 18, 2026
a6d21b9
Merge branch 'main' into fix-gh-153970-calledprocesserror-none
fedonman Jul 18, 2026
fa1a5d6
gh-153970: Align test and NEWS with the f-string message
fedonman Jul 18, 2026
da53d9b
Merge branch 'fix-gh-153970-calledprocesserror-none' of github.com:fe…
fedonman Jul 18, 2026
8dbfbe3
Merge branch 'main' into fix-gh-153970-calledprocesserror-none
fedonman Jul 21, 2026
f3bd991
gh-153970: Document that CalledProcessError.returncode is an integer
fedonman Jul 22, 2026
5dffa29
Merge branch 'main' into fix-gh-153970-calledprocesserror-none
fedonman Jul 22, 2026
19a86c2
gh-153970: Validate the returncode type in CalledProcessError
fedonman Jul 22, 2026
46d6a68
Merge branch 'main' into fix-gh-153970-calledprocesserror-none
fedonman Jul 22, 2026
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
8 changes: 6 additions & 2 deletions Doc/library/subprocess.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,8 +236,8 @@ underlying :class:`Popen` interface can be used directly.

.. attribute:: returncode

Exit status of the child process. If the process exited due to a
signal, this will be the negative signal number.
Exit status of the child process, an integer. If the process
exited due to a signal, this will be the negative signal number.

.. attribute:: cmd

Expand All @@ -260,6 +260,10 @@ underlying :class:`Popen` interface can be used directly.
.. versionchanged:: 3.5
*stdout* and *stderr* attributes added

.. versionchanged:: 3.16
The constructor now raises :exc:`TypeError` if *returncode* is
neither an integer nor ``None``.


.. _frequently-used-arguments:

Expand Down
10 changes: 8 additions & 2 deletions Lib/subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,18 @@ class CalledProcessError(SubprocessError):
cmd, returncode, stdout, stderr, output
"""
def __init__(self, returncode, cmd, output=None, stderr=None):
if returncode is not None and not isinstance(returncode, int):

@vstinner vstinner Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I dislike raising a new exception, since CalledProcessError is created when we are already handling another exception. CalledProcessError should accept anything for returncode, and then does it best to convert returncode to a string in __str__().

raise TypeError(
"returncode must be an integer or None, not "
f"{type(returncode).__name__}")
self.returncode = returncode
self.cmd = cmd
self.output = output
self.stderr = stderr

def __str__(self):
# A None returncode is falsy, so it skips the '< 0' comparison
# below and is formatted by the final branch instead.

@vstinner vstinner Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

returncode should not be None, so I would prefer to omit this comment.

if self.returncode and self.returncode < 0:
try:
return "Command %r died with %r." % (
Expand All @@ -151,8 +157,8 @@ def __str__(self):
return "Command %r died with unknown signal %d." % (
self.cmd, -self.returncode)
else:
return "Command %r returned non-zero exit status %d." % (
self.cmd, self.returncode)
return (f"Command {self.cmd!r} returned non-zero "
f"exit status {self.returncode}.")

@vstinner vstinner Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This two lines changes is the only needed change IMO.

@picnixz picnixz Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But then we are still having the possibility of the TypeError. In this case I would rather just document that we need an error code that is an integer and leave it as is.

@vstinner vstinner Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But then we are still having the possibility of the TypeError

Would you mind to elaborate which operation would raise TypeError? The issue is about str() and the change above does fix str().

With the change above, CalledProcessError can be called with any returncode, it just works.

@picnixz picnixz Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The issue with <0 branch. If you pass [1] you get a type error there

@vstinner vstinner Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh I'm sorry, I completely missed the error on self.returncode < 0.

Well, in that case, I suggest replacing:

if self.returncode and self.returncode < 0:

with:

if isinstance(self.returncode, int) and self.returncode < 0:

IMO we should accept any returncode value in the CalledProcessError constructor and str() should not fail.

@picnixz picnixz Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's slightly awkward for me to accept arbitrary return codes. I would assume them to be integers, and not None or other things, and would rather change the docs. In the first place, the class signature is not exposed so I don't consider it as part of the public API, whether subclassing or not.

But, if you really want to support arbitrary types, I'm ok with this change (+ the __repr__ one), but I really think it's a bit too niche.

@vstinner vstinner Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's slightly awkward for me to accept arbitrary return codes.

We can get a type other than int if there is a bug in subprocess under specific conditions. In that case, I would prefer to be able to create CalledProcessError which stores the full process state (returncode, stdout, stderr), rather than raising a new exception (ex: TypeError) which removes all subprocess context. Running a subprocess is an expensive operation, and you might not able to get the same stdout/stderr if you run the same command twice which would make debugging way harder.

You can also get a non-int returncode when mocking the Popen object. Example:

import unittest.mock
import subprocess

with unittest.mock.patch.object(subprocess, 'Popen'):
    subprocess.check_call(['true'])

Output:

Traceback (most recent call last):
  File "/home/vstinner/python/main/x.py", line 5, in <module>
    subprocess.check_call(['true'])
    ~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
  File "/home/vstinner/python/main/Lib/subprocess.py", line 534, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: <exception str() failed>

@picnixz picnixz Jul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, the mocking argument convinces me.


@property
def stdout(self):
Expand Down
2 changes: 1 addition & 1 deletion Lib/test/test_script_helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ def tearDown(self):
def test_interpreter_requires_environment_true(self, mock_check_call):
with mock.patch.dict(os.environ):
os.environ.pop('PYTHONHOME', None)
mock_check_call.side_effect = subprocess.CalledProcessError('', '')
mock_check_call.side_effect = subprocess.CalledProcessError(1, 'cmd')
self.assertTrue(script_helper.interpreter_requires_environment())
self.assertTrue(script_helper.interpreter_requires_environment())
self.assertEqual(1, mock_check_call.call_count)
Expand Down
14 changes: 14 additions & 0 deletions Lib/test/test_subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2449,6 +2449,20 @@ def test_CalledProcessError_str(self):
err = subprocess.CalledProcessError(-9876543, "fake cmd")
self.assertEqual(str(err), "Command 'fake cmd' died with unknown signal 9876543.")

# None return code (must not raise TypeError from __str__ itself)
err = subprocess.CalledProcessError(None, "fake cmd")
self.assertEqual(str(err), "Command 'fake cmd' returned non-zero exit status None.")

def test_CalledProcessError_returncode_type(self):
# returncode must be an integer or None; other types are rejected
# at construction so that __str__ cannot fail later on.
subprocess.CalledProcessError(1, "fake cmd")
subprocess.CalledProcessError(None, "fake cmd")
for returncode in ("1", 1.5, [1], object()):
with self.subTest(returncode=returncode):
with self.assertRaises(TypeError):
subprocess.CalledProcessError(returncode, "fake cmd")

def test_preexec(self):
# DISCLAIMER: Setting environment variables is *not* a good use
# of a preexec_fn. This is merely a test.
Expand Down
4 changes: 4 additions & 0 deletions Misc/NEWS.d/next/Library/2026-07-22-12-00-00.gh-issue-153970.Kq7Wn3.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Calling :func:`str` on a :exc:`subprocess.CalledProcessError` whose
:attr:`!returncode` is ``None`` no longer raises :exc:`TypeError`. The
constructor now also raises :exc:`TypeError` when *returncode* is neither
an integer nor ``None``.
Loading