gh-153970: Validate the returncode type in CalledProcessError

[fedonman]

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

The constructor now raises a clear TypeError when returncode is neither an integer nor None, so __str__ can never be handed a value it cannot format. Its else branch also uses an f-string, so a None returncode renders as "Command ... returned non-zero exit status None." instead of crashing. A test that built the exception with an empty-string returncode is updated to pass an integer, which is what a real failed command produces.

The constructor check is a behavior change, so it is documented with a .. versionchanged:: 3.16 note. returncode is a public attribute, so assigning a bad value after construction and then calling str() can still fail, but guarding every __str__ call would be the kind of defensive code the stdlib avoids.

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 #33706459 | 📁 Comparing 46d6a68 against main (0eac28e)

  🔍 Preview build  

5 files changed · ± 5 modified

± Modified

• library/csv.html
• library/subprocess.html
• using/index.html
• using/mac.html
• whatsnew/changelog.html