Pathological case segmentation fault in issubclass

[omnifarious]

BPO	858016
Nosy	@tim-one, @brettcannon
Files	flat_tuple.diff: Requires tuples for isinstance and issubclass to be flat recursion_check.diff: Checks recursion depth; diff against 2.3 branch

^{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/brettcannon'
closed_at = <Date 2004-03-20.23:02:45.000>
created_at = <Date 2003-12-11.03:13:04.000>
labels = ['interpreter-core']
title = 'Pathological case segmentation fault in issubclass'
updated_at = <Date 2004-03-20.23:02:45.000>
user = 'https://bugs.python.org/omnifarious'

bugs.python.org fields:

activity = <Date 2004-03-20.23:02:45.000>
actor = 'brett.cannon'
assignee = 'brett.cannon'
closed = True
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2003-12-11.03:13:04.000>
creator = 'omnifarious'
dependencies = []
files = ['1139', '1140']
hgrepos = []
issue_num = 858016
keywords = []
message_count = 13.0
messages = ['19382', '19383', '19384', '19385', '19386', '19387', '19388', '19389', '19390', '19391', '19392', '19393', '19394']
nosy_count = 3.0
nosy_names = ['tim.peters', 'brett.cannon', 'omnifarious']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue858016'
versions = ['Python 2.3']

[omnifarious]

This works for the PowerPC Python compiled with gcc 3.3
on OS X using fink. I suspect it's broader based than
that, but I don't have the ability to check properly.

Here's how to make it segment fault:

x = (basestring,)
for i in xrange(0, 1000000):
   x = (x,)
issubclass(str, x)

At least, it segment faults at the interactive prompt
this way. I don't know if it does when it's executed
from a file.

[omnifarious]

Logged In: YES
user_id=313

I forgot this:

Python 2.3.2 (#1, Dec 4 2003, 09:13:58)
[GCC 3.3 20030304 (Apple Computer, Inc. build 1493)] on darwin
Type "help", "copyright", "credits" or "license" for more
information.

[brettcannon]

Logged In: YES
user_id=357491

If you look at Object/abstract.c (line 2119 or so) for 2.4 CVS you
will notice that PyObject_IsSubclass goes into a 'for' loop for each
item in the tuple passed in and calls PyObject_IsSubclass .
Unfortunately it makes no check for whether the argument it is
passing is a class itself or not. This allows it to keep making calls
as long as the second argument is either a class or a tuple. This
is what is leads to the stack being blown and the subsequent
segfault.

Obvious solution is to put in a check that the argument about to be
passed is a class itself so as to not have such a deep call chain.
But since help(issubclass) actually makes the above use legit
(it says using a tuple as a second argument is equivalent as
passing each item to issubclass which is what it is doing, albeit in
a rather uncommon and pointless way), is it worth putting the
check in? Since this is such an obvious mis-use, I say no. But if
someone else on python-dev steps in and says otherwise I will
patch it.

[omnifarious]

Logged In: YES
user_id=313

Well, I think any case where the system segment faults
unexpectedly is bad, regardless of how pathological it is.

Personally, I think that issubclass should either have a
recursion limit after which it throws an exception, or it
shouldn't go into sub-tuples at all.

The reason I made this test is that I read the description
of the behavior of issublcass and found it rather strange,
so I decided to push it to see how far it would go.

[tim-one]

Logged In: YES
user_id=31435

Yes, this needs to be fixed if it *can* be fixed without heroic
effort or insane slowdown. Looks like it can be.

Brett, the missing piece of your worldview <wink> here is that
anywhere Python can be tricked into segfaulting is a kind
of "security hole" -- it's not just mistakes we want to protect
programmers from, we also want to bulletproof against hostile
users, to the extent sanely possible.

BTW, if issubclass() has this insecurity, I bet isinstance()
does too (they were introduced & coded at the same time).

[brettcannon]

Logged In: YES
user_id=357491

OK, consider my worldview fixed. =)

I will add a check in the tuple unpacking 'for' loop to make sure it
is only passing issubclass classes and not more tuples. Simple
and shouldn't break very much code. Otherwise the code would
have to keep a count and extra bookkeeping and it would get
messy quickly.

And I will take a look at isinstance, although this tuple feature was
added in 2.3 for issubclass so it might not be an issue.

And I will backport it.

[tim-one]

Logged In: YES
user_id=31435

I'm afraid that changing semantics needs to be run through
the python-dev wringer first -- "shouldn't break very much
code" isn't "shouldn't break any code". The *comments* in
these functions make it appear that they never intended to
support the OP's original code snippet, but the docs don't
match. This leaves the intent a mystery, so it needs to be
discussed.

[brettcannon]

Logged In: YES
user_id=357491

I have appended a file that adds a basic test to make sure that
when the items of a tuple are used to call isinstance or issubclass
that only classes or types are used; everything else raises
TypeError. Also tried to clarify the wording of the doc strings.
Changed the docs for isinstance since it allowed nested tupling
while issubclass' didn't.

Have a look and if someone will sign off on it I will apply the patch
and then start working on a 2.3 solution that doesn't break
semantics.

And I just realized I left out a Misc/NEWS in the patch; it will be
there when it gets applied.

[tim-one]

Logged In: YES
user_id=31435

Oh, Brett, you're missing a chance for some fun here! A bug
should always be assigned to the next person who should "do
something" about it. Think of it as a hot potato. You should
assign the bug to who you *want* to review this, and then
sit back and watch the fun, as each person in turn tries to
unload the potato onto someone else. That's one way to get
a comforting illusion of activity here <wink>.

[brettcannon]

Logged In: YES
user_id=357491

OK, Tim, start hopping. =)

[tim-one]

Logged In: YES
user_id=31435

Well, that wasn't as much fun for you as I hoped -- the
report just sat there until I got a free holiday <wink>. Marked
Accepted and back to you -- looks good!

[brettcannon]

Logged In: YES
user_id=357491

Have a patch for Python 2.3 that limits the depth of the tuples to the
recursion limit of the interpreter. That will definitely be used for 2.3
Question is which solution to use for 2.4 .

[brettcannon]

Logged In: YES
user_id=357491

Fixed in both Python 2.3 and 2.4 so that the depth of the tuple cannot
exceed the the recursion limit of the interpreter.