subprocess: `CalledProcessError.__str__` crashes when returncode is None

[fedonman]

Calling str() on a CalledProcessError crashes if its returncode is None.

The __str__ method uses if self.returncode and self.returncode < 0 for the "died with a signal" case, and otherwise formats the return code with %d. When returncode is None, that first check is falsy, so it falls through to the %d branch, and %d can't format None, so it raises TypeError. Having the exception's own string representation blow up is a pretty bad way to fail.

import subprocess
err = subprocess.CalledProcessError(None, "cmd")
str(err)  # TypeError: %d format: a real number is required, not NoneType

The fix is to check for returncode is None before the %d branch and return a plain message instead.

Found while going through devdanzin's audit of the standard library, item 9: https://gist.github.com/devdanzin/3198710e3c0128fda5e0a7b4e0768e5f

Linked PRs

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

[picnixz]

I'm not entirely sure you're meant to create CalledProcessError yourself. The docs say:

Subclass of SubprocessError, raised when a process run by check_call(), check_output(), or run() (with check=True) returns a non-zero exit status

So I don't thinkt he constructor is meant to be part of the public API. Note that the constructor is not documented either.

[fedonman]

@picnixz Fair point, and you're right that the constructor isn't documented, and that subprocess itself always builds this with a real integer return code.

The reasoning is less "people construct this directly" and more that an exception's __str__ shouldn't itself raise. CalledProcessError and its returncode attribute are public and documented, and exceptions do get constructed and re-raised by test code and by third-party libraries that imitate subprocess. If one of those ends up with returncode=None and propagates, the traceback machinery calls str() on it and you get a confusing chained TypeError that hides the real error instead of a readable message.

[devdanzin]

I'm not entirely sure you're meant to create CalledProcessError yourself.

Me neither, but if someone is creating a new subclass they could benefit from not having an admittedly hard to hit footgun. E.g.:

class StrictCompletedProcess(subprocess.CompletedProcess):
    def check_returncode(self):
        if self.returncode != 0:          # looks more explicit/correct...
            raise subprocess.CalledProcessError(
                self.returncode, self.args, self.stdout, self.stderr)

cp = StrictCompletedProcess(args=["mycommand"], returncode=None)
cp.check_returncode()

[picnixz]

I'm still unsure about this. In this case, I would rather leave it as is because it's already documented as being an exit code, so an integer. We don't do overly defensive programming in the stdlib.

[fedonman]

Should I close the issue?

[picnixz]

I personally think it's not relevant here. We can improve the documentation though, but I don't see a reason for raising a ValueError in __str__ for None exit codes. If such code can be legitimately obtained by the public API, then we ought to fix it, sure. But such fix should not raise a ValueError either and rather present a different __str__.

cc @gpshead

[picnixz]

If one of those ends up with returncode=None and propagates, the traceback machinery calls str() on it and you get a confusing chained TypeError that hides the real error instead of a readable message.

FTR, we would still consider a GIGO case. The exception would still point at the __str__ method so it wouldn't help for much more for debugging either.

[picnixz]

After looking at Victor's suggestion on the PR, I think we can just use %s instead of %d (or an f-string) for the return code. It would make it more friendly while allowing arbitrary types.