pickle: pure-Python `_Unpickler` mangles a constructor `TypeError`

[fedonman]

The pure-Python pickle._Unpickler mishandles a constructor failure while unpickling old-style instances (the INST and OBJ opcodes). If the class constructor raises TypeError, _instantiate does:

raise TypeError("in constructor for %s: %s" %
                (klass.__name__, str(err)), err.__traceback__)

Passing err.__traceback__ as a second positional argument puts the traceback object into the exception's args, so it leaks into str(), and there is no real chaining (__cause__ stays None).

import pickle, io
class Bad:
    def __init__(self, *a):
        raise TypeError("boom")
u = pickle._Unpickler(io.BytesIO())
try:
    u._instantiate(Bad, (1,))
except TypeError as e:
    print(e.args)       # ('in constructor for Bad: boom', <traceback object ...>)
    print(e.__cause__)  # None

The C _pickle unpickler does not wrap the error at all, it just lets the original TypeError propagate, so this only affects the pure-Python implementation.

The fix is to drop the wrapper and let the original TypeError propagate:

value = klass(*args)

The wrapper was added in 743d17e (1998) to name the class when __getinitargs__
returned something bogus. The constructor error already names the class in that case, so
the prefix only repeats it:

in constructor for WrongArgCount: WrongArgCount.__init__() takes 2 positional arguments but 3 were given

The traceback in args is a leftover from the Python 2 way of carrying one. It comes from
26d95c3, where raise T, V, tb became raise T(V, tb) instead of
raise T(V).with_traceback(tb). gh-102799 later changed sys.exc_info()[2] to
err.__traceback__ and nothing else.

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

Linked PRs

• gh-154002: Do not wrap a constructor TypeError in pickle._Unpickler #154003

[serhiy-storchaka]

The err.__traceback__-in-args leak is real, but before fixing it with ... from err, it's worth asking whether the wrapper should stay at all.

Its purpose (from Guido, 1998 — 743d17e) was to add the class name to a constructor TypeError. That's now redundant: a bad __getinitargs__ — the case it targeted — already produces OldStyleBogus.__init__() takes 2 positional arguments but 3 were given. The prefix just repeats a name that's already there. The only case it adds anything is a TypeError raised inside the constructor body, where the class is still visible in the traceback anyway.

So the simplest fix may be to drop the wrapper and let the original TypeError propagate. That also matches the C _pickle unpickler, which never wrapped it — closing the Python/C divergence instead of only fixing the Python side.

(Side note: the tb-in-args bug isn't from gh-102799; it dates to the Py3 conversion 26d95c3, where raise T, V, tb became raise T(V, tb) instead of raise T(V).with_traceback(tb). gh-102799 only renamed the tb expression.)

[fedonman]

@serhiy-storchaka Good call, dropped the wrapper.

You are right that the prefix is redundant. For the __getinitargs__ case it actually repeats the name twice:

in constructor for WrongArgCount: WrongArgCount.__init__() takes 2 positional arguments but 3 were given

Both unpicklers now give the same error for a bad argument count and for a TypeError raised in the constructor body, so the divergence is gone too.

You are also right about the provenance. It comes from 26d95c3, where raise T, V, tb became raise T(V, tb) instead of raise T(V).with_traceback(tb). gh-102799 only changed sys.exc_info()[2] to err.__traceback__.

GH-154003 is updated and ready for review.