gh-153970: Fix str() of CalledProcessError when returncode is not an integer

[fedonman]

CalledProcessError.returncode holds a process exit status, which subprocess always sets to an integer. Calling str() on one whose returncode was None raised TypeError, because the __str__ else branch formats the value with %d. A returncode of another type, such as a string or a list, failed even earlier at the self.returncode < 0 comparison.

__str__ now guards that comparison with isinstance(self.returncode, int) and formats the exit status with an f-string, so it works for any value. The constructor is left as it was, so the exception still carries the full process state (returncode, output, stderr) even when the returncode is not an integer. Running a subprocess is expensive and you may not get the same output twice, so losing that context to a second exception would make debugging harder.

One way to hit this is a mocked Popen:

import unittest.mock
import subprocess

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

That used to end in subprocess.CalledProcessError: <exception str() failed>. Now it prints the command and the mock object as the exit status.

The docs for the returncode attribute now say it is an integer.

An earlier version of this PR validated the returncode type in the constructor and raised TypeError for anything else. That is dropped, along with its test and doc note, following the review discussion.

Found as item 9 in devdanzin's standard library audit: https://gist.github.com/devdanzin/3198710e3c0128fda5e0a7b4e0768e5f

Fixes #153970.

• Issue: subprocess: CalledProcessError.__str__ crashes when returncode is None #153970

[picnixz]

See my reservations on the issue; I don't think the constructor is a public API and thus it's a case of GIGO (garbage-in, garbage-out)

[fedonman]

See my reservations on the issue; I don't think the constructor is a public API and thus it's a case of GIGO (garbage-in, garbage-out)

Thank you for reviewing this. Answered on the Issue thread.

[vstinner]

I don't think that there is a reason to format the returncode using %d. The issue can be addressed by using a f-string, without checking self.returncode:

diff --git a/Lib/subprocess.py b/Lib/subprocess.py
index 6fe2ec98fb4..df23fb18a51 100644
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -151,8 +151,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}.")
 
     @property
     def stdout(self):

[picnixz]

I guess this patch would be acceptable indeed.

[fedonman]

Thank you. I implemented the requested changes.

[picnixz]

Actually, after sleeping a bit, we would still have an issue if someone is using a returncode as a string for instance (just because self.returncode < 0 would raise here). Since we are considering only theoretical usages, I really wonderf whether we need the change.

This change would make None supported, but would still fail on truthy returncodes not comparable to 0.

So... I'd say:

• either check returncode type in the constructor
• do nothing but reword the docs a bit.

[read-the-docs-community]

Documentation build overview

📚 cpython-previews | 🛠️ Build #33765189 | 📁 Comparing fdad6bd against main (0eac28e)

  🔍 Preview build  

15 files changed · ± 15 modified

± Modified

• license.html
• library/asyncio-eventloop.html
• library/csv.html
• library/ctypes.html
• library/difflib.html
• library/exceptions.html
• library/logging.handlers.html
• library/msvcrt.html
• library/stdtypes.html
• library/subprocess.html
• and 5 more...

[vstinner]

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__().

[vstinner]

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

[vstinner]

This two lines changes is the only needed change IMO.

[picnixz]

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]

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]

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

[vstinner]

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]

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]

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]

Ok, the mocking argument convinces me.

[vstinner]

Since we are considering only theoretical usages, I really wonderf whether we need the change.

The bug has been found by fuzzing. If we don't fix the issue, someone else will come back in a few months with the same issue.

[fedonman]

Addressed comments.