Avoid calling tp_compare with different types

[ddorfman]

BPO	995939
Nosy	@arigo
Files	coerce.diff: try_3way_compare change and test case

^{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 = None
closed_at = <Date 2004-12-23.22:14:10.000>
created_at = <Date 2004-07-22.14:14:24.000>
labels = ['interpreter-core']
title = 'Avoid calling tp_compare with different types'
updated_at = <Date 2004-12-23.22:14:10.000>
user = 'https://bugs.python.org/ddorfman'

bugs.python.org fields:

activity = <Date 2004-12-23.22:14:10.000>
actor = 'arigo'
assignee = 'none'
closed = True
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2004-07-22.14:14:24.000>
creator = 'ddorfman'
dependencies = []
files = ['6105']
hgrepos = []
issue_num = 995939
keywords = ['patch']
message_count = 5.0
messages = ['46424', '46425', '46426', '46427', '46428']
nosy_count = 2.0
nosy_names = ['arigo', 'ddorfman']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue995939'
versions = []

[ddorfman]

C implementations of the tp_compare slot usually expect both of their
arguments to have the type for which that slot is defined (this isn't
documented anywhere that I can find, but many core types assume this,
and extension authors are likely to do the same). A problem occurs if a
user-defined nb_coerce slot (coerce) returns objects with different
types. Avoid the problem by refusing to call a non-Python tp_compare
unless the arguments have the same type. Indiscriminately calling
tp_compare with different types is wrong as long as there are
implementations that don't check the type of the second argument, but
Python implementations should be allowed to receive a different type.

Other options for fixing the problem:

• Make PyNumber_CoerceEx require that the results have the same
  type. This would prevent a user-defined __coerce__ from
  communicating an arbitrary object to a user-defined __cmp__.
  Furthermore, there are uses for coerce besides comparison, and those
  uses might not require identical types.
• Change tp_compare implementations to check the type of the
  second argument. This might be the most desirable long-term solution,
  but as I understand it, tp_richcompare is preferred over tp_compare for
  new code, and old (current) code is likely assuming that the types are
  the same.

Addresses bug bpo-980352

[arigo]

Logged In: YES
user_id=4771

Is your patch complete? There seem to be a lot of places in object.c that call tp_compare slots. Shouldn't we try to make sure that none of these places can ever be called with two different types?

It also looks like we have to do something special about a value of &_PyObject_SlotCompare in tp_compare, because this function from typeobject.c checks itself the type of its arguments and does specific things with them. This should be thought about.

Moreover there is the issue of the user subclassing built-in types, e.g. PyInt_Type.tp_compare is capable of comparing different user subclasses of "int" for each other.

[ddorfman]

Logged In: YES
user_id=908995

The patch is intended to be complete, but it's certainly possible that I
missed something. Every other call to tp_compare checks that the
operands have the same type or the same compare function; the only
exceptions are if one of operands passes PyInstance_Check or if it's
_PyObject_SlotCompare (it's treated specially in try_3way_compare).

I'm actually more concerned about restricting too much than having
missing something. The tests I wrote try to make sure that the
user-visible changes are limited to the absence of a crash, but I'm not
sure that I didn't accidently break some code that used to work.

Comparing subclasses of builtins to builtins still works, too. On line 599
of the post-patch object.c, we see:

	/* If both have the same (non-NULL) tp_compare, use it. */
	if (f != NULL && f == w->ob_type->tp_compare) {
		c = (*f)(v, w);
		return adjust_tp_compare(c);
	}

which takes care of that case.

[arigo]

Logged In: YES
user_id=4771

Sorry, I did not read well enough what object.c was doing.
I agree now that your patch should protect again all
crashes, and that the concern is whether it is too restrictive.

Before rich comparison, there was no obvious sane way to
directly compare objects of different types in C extension
types. I remember having the problem back in Python 1.5.2;
at the time I solved the problem with an nb_coerce that
returned "fake" objects of a common type, and a tp_compare
that decoded these fake objects. Now I see that it would
have been simpler to solve the problem by either:

1. writing a nb_coerce that doesn't do anything (but which
   is implemented; otherwise tp_compare don't get called at all).
   This is what would be broken by the present patch.

2. easier yet, just making sure that both types have the
   same tp_compare slot. We don't need nb_coerce at all in
   this case. The tp_compare would check for the type of both
   arguments anyway. The present patch wouldn't break that.

There is no other reasonable safe way that I can of to fix
the problem (1)... But admitedly the whole issue is a bit
far-fetched. So I'd suggest that the issue is deferred
until Python 2.4 is out, and then we put the patch into the
pre-2.5 CVS head. It's a bit late in the 2.4 release
process to risk breaking potential C extension modules.

A note about the patch: in the first comment you say that v
and w have different types, but that's potentially wrong --
they could have the same type but no tp_compare slot.

The new single test introduced after PyNumber_CoerceEx()
might be too restrictive: it would be enough if v and w have
the same tp_compare slot after coercion. Pushing things a
bit over the edge of sanity, we might ask "what if coercion
returns old-style instances, or something with
_PyObject_SlotCompare?". So maybe we should try the whole
try_3way_compare again after coercion (without of course
trying to coerce more than once!).

[arigo]

Logged In: YES
user_id=4771

I added a minor change after the coercion, to accept two
objects not necessarily of the same type but with the same
tp_compare.

Applied as planned in the 2.5 CVS as:

[Objects/object.c](https://github.com/python/cpython/blob/main/Objects/object.c)  rev 2.225
[Lib/test/test_coercion.py](https://github.com/python/cpython/blob/main/Lib/test/test_coercion.py)  rev 1.8