subprocess.Popen.__del__ raises AttributeError if __init__ was called with an invalid argument list

[chortos]

BPO	12085
Nosy	@gvanrossum, @terryjreedy, @pitrou, @vstinner, @bitdancer, @akheron, @serhiy-storchaka
Files	_child_created.diff _child_created_2.diff _child_created_3.diff subprocess_del.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2014-02-10.17:28:32.927>
created_at = <Date 2011-05-16.03:08:55.096>
labels = ['type-bug', 'library']
title = 'subprocess.Popen.__del__ raises AttributeError if __init__ was called with an invalid argument list'
updated_at = <Date 2014-02-10.17:28:32.926>
user = 'https://bugs.python.org/chortos'

bugs.python.org fields:

activity = <Date 2014-02-10.17:28:32.926>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2014-02-10.17:28:32.927>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2011-05-16.03:08:55.096>
creator = 'chortos'
dependencies = []
files = ['22007', '22123', '22156', '31849']
hgrepos = []
issue_num = 12085
keywords = ['patch']
message_count = 31.0
messages = ['136064', '136938', '136939', '136940', '136941', '136942', '136948', '136949', '136950', '136952', '136964', '137025', '137091', '137399', '137403', '197748', '197749', '197754', '198225', '198227', '198228', '198229', '198230', '198231', '198232', '198233', '198235', '198322', '198333', '210852', '210853']
nosy_count = 10.0
nosy_names = ['gvanrossum', 'terry.reedy', 'pitrou', 'vstinner', 'Arfrever', 'r.david.murray', 'chortos', 'python-dev', 'petri.lehtinen', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue12085'
versions = ['Python 2.7', 'Python 3.3', 'Python 3.4']

[chortos]

If subprocess.Popen is called with a keyword argument whose name is undefined or simply too many arguments, an instance of the Popen class is created but its __init__ method call fails due to the invalid argument list. (Immediately) afterwards, the new instance is destroyed and its __del__ method is called, which checks the _child_created field that is defined by __init__; but __init__ never got the chance to execute, so the field is not defined, and __del__ raises an AttributeError, which is written out to stderr.

>>> subprocess.Popen(fdsa=1)
Exception AttributeError: "'Popen' object has no attribute '_child_created'" in <bound method Popen.__del__ of <subprocess.Popen object at 0x1006ee790>> ignored
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __init__() got an unexpected keyword argument 'fdsa'
>>> subprocess.Popen((), 0, None, None, None, None, None, True, False, None, None, False, None, 0, True, False, (), None)
Exception AttributeError: "'Popen' object has no attribute '_child_created'" in <bound method Popen.__del__ of <subprocess.Popen object at 0x1006ee790>> ignored
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __init__() takes at most 18 positional arguments (19 given)

I encountered this while trying to write a piece of code compatible with Python 3.2 and earlier versions simultaneously. The subprocess module in Python 3.2 adds a new argument to the constructor of Popen, pass_fds, while changing the default value of another, close_fds, from False to True. In order to utilize pass_fds, I tried code that looked like this:

try:
    process = Popen(*args, pass_fds=myfds, **kwargs)
except TypeError:
    process = Popen(*args, **kwargs)

It worked but printed a message about the exception in __del__, which interfered with my own output. Without delving too much into the details of my code, I ended up just passing close_fds=False.

The attached patch avoids the exception by converting the reference to _child_created in __del__ into a three-argument getattr call.

[akheron]

IMO it should be explained in a comment why getattr is used instead of just self._child_created. Otherwise the patch looks good and useful.

[vstinner]

We can use a class attribute to set the attribute before calling __init__:

diff --git a/Lib/subprocess.py b/Lib/subprocess.py
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -664,6 +664,8 @@ _PLATFORM_DEFAULT_CLOSE_FDS = object()
 
 
 class Popen(object):
+    _child_created = False
+
     def __init__(self, args, bufsize=0, executable=None,
                  stdin=None, stdout=None, stderr=None,
                  preexec_fn=None, close_fds=_PLATFORM_DEFAULT_CLOSE_FDS,

[akheron]

STINNER Victor wrote:

We can use a class attribute to set the attribute before calling __init__:

True, this is clever. Will you commit?

[vstinner]

True, this is clever. Will you commit?

I'm not sure that it's the pythonic way to solve such problem. Can you work on a patch including a test?

[akheron]

I'm not sure that it's the pythonic way to solve such problem. Can you work on a patch including a test?

Yeah, getattr() might be more Pythonic indeed.

How can this be tested? According to the initial report, exceptions raised in __del__ seem to be ignored.

[chortos]

We can use a class attribute to set the attribute before calling __init__

Ah, yes, I thought about adding a class attribute as well, but the class currently does not have any and initializes instance attributes to default values in __init__, so I chose getattr.

How can this be tested? According to the initial report, exceptions raised in __del__ seem to be ignored.

Exceptions raised in __del__ are printed to sys.stderr, even if it has been reassigned by Python code:

>>> import sys, io, subprocess
>>> sys.stderr = io.StringIO()
>>> subprocess.Popen(fdsa=1)
>>> sys.stderr.getvalue()
'Exception AttributeError: "\'Popen\' object has no attribute \'_child_created\'" in <bound method Popen.__del__ of <subprocess.Popen object at 0x1006ee710>> ignored\nTraceback (most recent call last):\n  File "<stdin>", line 1, in <module>\nTypeError: __init__() got an unexpected keyword argument \'fdsa\'\n'

test_generators.py already uses this trick in a test similar to what would be needed for this issue around line 1856.

Should I attempt to modify my patch to include a comment and a test?

[vstinner]

Should I attempt to modify my patch to include a comment and a test?

Yes please, you can use support.captured_stderr() tool.

[akheron]

Oleg Oshmyan wrote:

Should I attempt to modify my patch to include a comment and a test?

Absolutely, go ahead if it's not too much trouble.

[chortos]

Here is a new patch; please take a look. Do I understand correctly that I should now remove the old one?

[chortos]

Looking at my own patch, I've noticed that the comment I added to Popen.__del__ doesn't sound quite right to me in terms of language and doesn't end in a full stop while all other comments in the same method do. Am I being too picky? I can't really think of a wording that would sound any better without it also being awkward though.

[akheron]

I don't think you should remove the old patch, as it's a part of the discussion.

For the new patch, I'd explicitly state what can go wrong, e.g. add "(e.g. invalid parameters passed to __init__)" or something like that.

Otherwise, looks good.

[chortos]

I've added passing __init__ an undeclared keyword argument as an example to the comment following your suggestion. I've also reworded the comment to make it sound better to me and added a full stop. :-)

[python-dev]

New changeset 0f2714e49583 by Victor Stinner in branch '3.2':
Close bpo-12085: Fix an attribute error in subprocess.Popen destructor if the
http://hg.python.org/cpython/rev/0f2714e49583

New changeset 71dfd8cf4bf5 by Victor Stinner in branch 'default':
(Merge 3.2) Close bpo-12085: Fix an attribute error in subprocess.Popen destructor
http://hg.python.org/cpython/rev/71dfd8cf4bf5

New changeset 26ea0a46aadd by Victor Stinner in branch '2.7':
Close bpo-12085: Fix an attribute error in subprocess.Popen destructor if the
http://hg.python.org/cpython/rev/26ea0a46aadd