bpo-38005: Fixed comparing and creating of InterpreterID and ChannelID. #15652

serhiy-storchaka · 2019-09-02T17:06:39Z

Fix a crash in comparing with float (and maybe other crashes).
They are now never equal to strings and non-integer numbers.
Comparison with a large number no longer raises OverflowError.
Arbitrary exceptions no longer silenced in constructors and comparisons.
TypeError raised in the constructor contains now the name of the type.
Accept only ChannelID and int-like objects in channel functions.
Accept only InterpreterId, int-like objects and str in the InterpreterId constructor.
Accept int-like objects, not just int in interpreter related functions.

https://bugs.python.org/issue38005

* They are now never equal to strings and non-integer numbers. * Comparison with a large number no longer raises OverflowError. * Arbitrary exceptions no longer silenced in constructors and comparisons. * TypeError raised in the constructor contains now the name of the type.

serhiy-storchaka · 2019-09-02T17:11:56Z

Lib/test/test__xxsubinterpreters.py

        class Int(str):
            def __init__(self, value):
                self._value = value
            def __int__(self):
                return self._value

-        id = interpreters.InterpreterID(Int(10), force=True)
-        self.assertEqual(int(id), 10)
+        for id in ('10', b'10', bytearray(b'10'), memoryview(b'10'), '1_0',


It is a questionable behavior to me.

…rId constructor.

ericsnowcurrently

LGTM

Thanks for working on this! I'm fine with the change as-is (including dropping support for float & bytes), but have left comments where I'd like you to at least consider a few small changes.

ericsnowcurrently · 2019-09-13T13:01:18Z

Modules/_xxsubinterpretersmodule.c

-                                     "O|$pppp:ChannelID.__new__", kwlist,
-                                     &id, &send, &recv, &force, &resolve))
+                                     "O&|$pppp:ChannelID.__new__", kwlist,
+                                     channel_id_converter, &cid, &send, &recv, &force, &resolve))


That's a neat trick. :)

ericsnowcurrently · 2019-09-13T13:03:03Z

Modules/_xxsubinterpretersmodule.c

+    else if (PyLong_Check(other)) {
+        /* Fast path */
+        int overflow;
+        long long othercid = PyLong_AsLongLongAndOverflow(other, &overflow);


Use int64_t or the macro?

Using int64_t can cause loss of bits and returning wrong result if long long is larger than int64_t.

But using long long is good here. The comparison with cid->id will return false if the value is too large for int64_t. Overflow also returns false instead of raising an exception (added a new test for this).

ericsnowcurrently · 2019-09-13T13:03:28Z

Objects/interpreteridobject.c

+    else if (PyLong_CheckExact(other)) {
+        /* Fast path */
+        int overflow;
+        long long otherid = PyLong_AsLongLongAndOverflow(other, &overflow);


Use int64_t or the macro?

ericsnowcurrently · 2019-09-13T13:04:54Z

Lib/test/test__xxsubinterpreters.py

-                self._value = value
-            def __int__(self):
-                return self._value
+            def __index__(self):


__index__ makes sense, but what about __int__ too. Shouldn't both work?

float has __int__, but not __index__. (10.1).__int__() returns 10. So __int__ should not work.

Note that I changed return self._value to return 10 because int(10) and int(str(10)) has the same result. So it was not clear what way the str subclass with __int__ used, bot has the same result.

ericsnowcurrently · 2019-09-13T13:05:33Z

Lib/test/test__xxsubinterpreters.py

                    interpreters.InterpreterID(id)
+        for id in (-1, '-1', 'spam'):
+            with self.assertRaises(ValueError):


For such short loops it is better to rewrite tests in other way:

self.assertRaises(TypeError, interpreters.InterpreterID, 10.0) self.assertRaises(TypeError, interpreters.InterpreterID, b'10') self.assertRaises(TypeError, interpreters.InterpreterID, []) self.assertRaises(ValueError, interpreters.InterpreterID, -1) self.assertRaises(ValueError, interpreters.InterpreterID, '-1') self.assertRaises(ValueError, interpreters.InterpreterID, 'spam') self.assertRaises(OverflowError, interpreters.InterpreterID, 2**64)

The traceback would show what line is failed.

ericsnowcurrently · 2019-09-13T13:15:51Z

Lib/test/test__xxsubinterpreters.py

        with self.assertRaises(OverflowError):
            interpreters.InterpreterID(2**64)
-        with self.assertRaises(TypeError):
-            interpreters.InterpreterID(object())


It would be nice to have a test still for object().

I replaced it with []. But if you prefer, I'll return object().

ericsnowcurrently · 2019-09-13T13:16:09Z

Lib/test/test__xxsubinterpreters.py

@@ -572,6 +567,14 @@ def test_equality(self):
        self.assertTrue(id1 == id1)
        self.assertTrue(id1 == id2)
        self.assertTrue(id1 == int(id1))
+        self.assertTrue(int(id1) == id1)


serhiy-storchaka · 2019-09-13T14:16:20Z

Lib/test/test__xxsubinterpreters.py

-                self._value = value
-            def __int__(self):
-                return self._value
+            def __index__(self):


float has __int__, but not __index__. (10.1).__int__() returns 10. So __int__ should not work.

Note that I changed return self._value to return 10 because int(10) and int(str(10)) has the same result. So it was not clear what way the str subclass with __int__ used, bot has the same result.

serhiy-storchaka · 2019-09-13T14:27:22Z

Lib/test/test__xxsubinterpreters.py

                    interpreters.InterpreterID(id)
+        for id in (-1, '-1', 'spam'):
+            with self.assertRaises(ValueError):


For such short loops it is better to rewrite tests in other way:

self.assertRaises(TypeError, interpreters.InterpreterID, 10.0) self.assertRaises(TypeError, interpreters.InterpreterID, b'10') self.assertRaises(TypeError, interpreters.InterpreterID, []) self.assertRaises(ValueError, interpreters.InterpreterID, -1) self.assertRaises(ValueError, interpreters.InterpreterID, '-1') self.assertRaises(ValueError, interpreters.InterpreterID, 'spam') self.assertRaises(OverflowError, interpreters.InterpreterID, 2**64)

The traceback would show what line is failed.

serhiy-storchaka · 2019-09-13T14:28:33Z

Lib/test/test__xxsubinterpreters.py

        with self.assertRaises(OverflowError):
            interpreters.InterpreterID(2**64)
-        with self.assertRaises(TypeError):
-            interpreters.InterpreterID(object())


I replaced it with []. But if you prefer, I'll return object().

serhiy-storchaka · 2019-09-13T14:29:43Z

Lib/test/test__xxsubinterpreters.py

@@ -572,6 +567,14 @@ def test_equality(self):
        self.assertTrue(id1 == id1)
        self.assertTrue(id1 == id2)
        self.assertTrue(id1 == int(id1))
+        self.assertTrue(int(id1) == id1)
+        self.assertTrue(id1 == float(int(id1)))
+        self.assertTrue(float(int(id1)) == id1)


This one test crashed with the past implementation.

serhiy-storchaka · 2019-09-13T14:34:28Z

Modules/_xxsubinterpretersmodule.c

+    else if (PyLong_Check(other)) {
+        /* Fast path */
+        int overflow;
+        long long othercid = PyLong_AsLongLongAndOverflow(other, &overflow);


Using int64_t can cause loss of bits and returning wrong result if long long is larger than int64_t.

But using long long is good here. The comparison with cid->id will return false if the value is too large for int64_t. Overflow also returns false instead of raising an exception (added a new test for this).

serhiy-storchaka · 2019-09-13T14:41:46Z

Objects/interpreteridobject.c

+            pyid = idobj;
+            Py_INCREF(pyid);
+        }
+        else if (PyUnicode_Check(idobj)) {


I am not sure strings should be supported in constructor. The error message says that the argument must be an int. And the code can be simpler without supporting strings. What is the use case for this?

miss-islington · 2019-09-13T19:50:29Z

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

…D. (pythonGH-15652) * Fix a crash in comparing with float (and maybe other crashes). * They are now never equal to strings and non-integer numbers. * Comparison with a large number no longer raises OverflowError. * Arbitrary exceptions no longer silenced in constructors and comparisons. * TypeError raised in the constructor contains now the name of the type. * Accept only ChannelID and int-like objects in channel functions. * Accept only InterpreterId, int-like objects and str in the InterpreterId constructor. * Accept int-like objects, not just int in interpreter related functions. (cherry picked from commit bf16991) Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-bot · 2019-09-13T19:50:39Z

GH-16129 is a backport of this pull request to the 3.8 branch.

miss-islington · 2019-09-14T16:07:04Z

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington · 2019-09-14T16:07:06Z

Sorry @serhiy-storchaka, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker bf169915ecdd42329726104278eb723a7dda2736 3.8

…annelID. (pythonGH-15652) * Fix a crash in comparing with float (and maybe other crashes). * They are now never equal to strings and non-integer numbers. * Comparison with a large number no longer raises OverflowError. * Arbitrary exceptions no longer silenced in constructors and comparisons. * TypeError raised in the constructor contains now the name of the type. * Accept only ChannelID and int-like objects in channel functions. * Accept only InterpreterId, int-like objects and str in the InterpreterId constructor. * Accept int-like objects, not just int in interpreter related functions. (cherry picked from commit bf16991) Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-bot · 2019-09-14T16:09:33Z

GH-16145 is a backport of this pull request to the 3.8 branch.

…annelID. (GH-15652) (GH-16145) * Fix a crash in comparing with float (and maybe other crashes). * They are now never equal to strings and non-integer numbers. * Comparison with a large number no longer raises OverflowError. * Arbitrary exceptions no longer silenced in constructors and comparisons. * TypeError raised in the constructor contains now the name of the type. * Accept only ChannelID and int-like objects in channel functions. * Accept only InterpreterId, int-like objects and str in the InterpreterId constructor. * Accept int-like objects, not just int in interpreter related functions. (cherry picked from commit bf16991)

serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.8 labels Sep 2, 2019

serhiy-storchaka requested a review from ericsnowcurrently September 2, 2019 17:06

the-knights-who-say-ni added the CLA signed label Sep 2, 2019

bedevere-bot added the awaiting core review label Sep 2, 2019

serhiy-storchaka commented Sep 2, 2019

View reviewed changes

serhiy-storchaka added 6 commits September 8, 2019 14:19

Fix a crash: InterpreterID and ChannelID are not proper int subclasses.

6c28b76

Use a converter function and O&.

5524e62

Accept only ChannelID and int-like objects in channel functions.

b6bf9c3

Accept only InterpreterId, int-like objects and str in the Interprete…

f3603dd

…rId constructor.

Accept int-like objects, not just int in interpreter related functions.

b768077

Unify error messages.

052fc77

ericsnowcurrently approved these changes Sep 13, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Sep 13, 2019

serhiy-storchaka added 2 commits September 13, 2019 17:19

Merge branch 'master' into interpreter-id

c8219f9

Rewrite some tests in simpler form.

6d56a37

serhiy-storchaka commented Sep 13, 2019

View reviewed changes

serhiy-storchaka merged commit bf16991 into python:master Sep 13, 2019

serhiy-storchaka deleted the interpreter-id branch September 13, 2019 19:50

bedevere-bot removed the awaiting merge label Sep 13, 2019

bedevere-bot removed the needs backport to 3.8 label Sep 13, 2019

serhiy-storchaka added the needs backport to 3.8 label Sep 14, 2019

miss-islington assigned serhiy-storchaka Sep 14, 2019

bedevere-bot removed the needs backport to 3.8 label Sep 14, 2019

serhiy-storchaka removed their assignment Dec 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-38005: Fixed comparing and creating of InterpreterID and ChannelID. #15652

bpo-38005: Fixed comparing and creating of InterpreterID and ChannelID. #15652

serhiy-storchaka commented Sep 2, 2019 •

edited

Loading

serhiy-storchaka Sep 2, 2019

ericsnowcurrently left a comment

ericsnowcurrently Sep 13, 2019

ericsnowcurrently Sep 13, 2019

serhiy-storchaka Sep 13, 2019

ericsnowcurrently Sep 13, 2019

ericsnowcurrently Sep 13, 2019

serhiy-storchaka Sep 13, 2019

ericsnowcurrently Sep 13, 2019

serhiy-storchaka Sep 13, 2019

ericsnowcurrently Sep 13, 2019

serhiy-storchaka Sep 13, 2019

ericsnowcurrently Sep 13, 2019

serhiy-storchaka Sep 13, 2019

serhiy-storchaka Sep 13, 2019

serhiy-storchaka Sep 13, 2019

serhiy-storchaka Sep 13, 2019

serhiy-storchaka Sep 13, 2019

serhiy-storchaka Sep 13, 2019

miss-islington commented Sep 13, 2019

bedevere-bot commented Sep 13, 2019

miss-islington commented Sep 14, 2019

miss-islington commented Sep 14, 2019

bedevere-bot commented Sep 14, 2019

bpo-38005: Fixed comparing and creating of InterpreterID and ChannelID. #15652

bpo-38005: Fixed comparing and creating of InterpreterID and ChannelID. #15652

Conversation

serhiy-storchaka commented Sep 2, 2019 • edited Loading

Choose a reason for hiding this comment

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miss-islington commented Sep 13, 2019

bedevere-bot commented Sep 13, 2019

miss-islington commented Sep 14, 2019

miss-islington commented Sep 14, 2019

bedevere-bot commented Sep 14, 2019

serhiy-storchaka commented Sep 2, 2019 •

edited

Loading