gh-153970: Fix CalledProcessError.__str__ crash when returncode is None

[fedonman]

Calling str() on a CalledProcessError raised TypeError when its returncode was None. The __str__ method fell through to a branch that formats the return code with %d, and %d can't format None, so building the error message crashed.

This adds a check for returncode is None that returns "Command ... returned an unknown exit status." instead. The other cases, negative return codes for signals and positive ones for exit codes, are unchanged.

I extended the existing test_CalledProcessError_str test with a None case.

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.