Bug 13748 - pytalloc's tp_compare/tp_richcompare functions should return only -1, 0 or 1
Summary: pytalloc's tp_compare/tp_richcompare functions should return only -1, 0 or 1
Status: RESOLVED WONTFIX
Alias: None
Product: TALLOC
Classification: Unclassified
Component: libtalloc (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Simo Sorce
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-01-16 11:33 UTC by Adam Sampson
Modified: 2020-10-22 03:31 UTC (History)
0 users

See Also:


Attachments
Patch: use a wrapper function to limit the returned value (1.70 KB, patch)
2019-01-16 11:33 UTC, Adam Sampson
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Sampson 2019-01-16 11:33:25 UTC
Created attachment 14774 [details]
Patch: use a wrapper function to limit the returned value

In talloc 21.1.15 and earlier versions, pytalloc_default_cmp and pytalloc_base_default_cmp implement Python comparison functions for talloc objects. These functions return the difference between two pointers.

This is wrong for a couple of reasons:

- These functions are meant to return only -1, 0 or 1 (see https://docs.python.org/2/c-api/typeobj.html). Recent versions of the Python runtime system enforce this and produce a warning -- "tp_compare didn't return -1, 0 or 1" -- when they're used.

- The return value is an int, which is smaller than ptrdiff_t on some platforms.

I've attached a patch which fixes this using a wrapper function.
Comment 1 Douglas Bagnall 2020-10-22 02:52:42 UTC
Adam -- I think your patch *was* probably correct, but now that we don't support Python 2, and Python3 only has richcmp, not cmp, I think the correct thing is to remove this altogether. 

It is only referenced here:

#if PY_MAJOR_VERSION >= 3
        .tp_richcompare = pytalloc_default_richcmp,
#else
        .tp_compare = pytalloc_default_cmp,
#endif
};
Comment 2 Douglas Bagnall 2020-10-22 03:31:06 UTC
Just regarding the bug title which mentions Py#'s tp_richcompare: 
it is not affected, and returns a Python Boolean, like so:

        switch (op) {                                                                                                              
                case Py_EQ: return PyBool_FromLong(ptr1 == ptr2);                                                                  
                case Py_NE: return PyBool_FromLong(ptr1 != ptr2);                                                                  
                case Py_LT: return PyBool_FromLong(ptr1 < ptr2);                                                                   
                case Py_GT: return PyBool_FromLong(ptr1 > ptr2);                                                                   
                case Py_LE: return PyBool_FromLong(ptr1 <= ptr2);                                                                  
                case Py_GE: return PyBool_FromLong(ptr1 >= ptr2);                                                                  
        }