Skip to content

Conversation

onlined
Copy link
Contributor

@onlined onlined commented Mar 10, 2022

Description

In CPython, the id of an object is its address. It's computed by converting the pointer to an unsigned integer (PyLong_FromVoidPtr). A similar logic is present here, pointer is converted to a Py_ssize_t and CPyTagged_FromSsize_t is called with that integer.

There is a problem with that approach: Py_ssize_t cannot hold every pointer value. Sometimes overflow happens and CPyTagged_FromSsize_t is called with a negative integer.

With the new approach, the number is checked: If it fits in a Py_ssize_t, CPyTagged_FromSsize_t is called. If not, it is directly
converted to a PyObject using PyLong_FromVoidPtr.

Fixes #11148.

Test Plan

I used the Docker test in #11148. I could reproduce failure before the changes, it passed after the changes.

@onlined onlined changed the title Fix overflow in id function (CPyTagged_Id) mypyc: Fix overflow in id function (CPyTagged_Id) Mar 10, 2022
In CPython, the id of an object is its address. It's computed by
converting the pointer to an unsigned integer (PyLong_FromVoidPtr). A
similar logic is present here, pointer is converted to a Py_ssize_t and
CPyTagged_FromSsize_t is called with that integer.

There is a problem with that approach: Py_ssize_t cannot hold every
pointer value. Sometimes overflow happens and CPyTagged_FromSsize_t is
called with a negative integer.

With the new approach, the number is checked: If it fits in a
Py_ssize_t, CPyTagged_FromSsize_t is called. If not, it is directly
converted to a PyObject using PyLong_FromVoidPtr.
@onlined onlined force-pushed the mypyc-id-fix-overflow branch from f88cc57 to 518c864 Compare March 10, 2022 19:21
@jhance
Copy link
Collaborator

jhance commented Mar 10, 2022

Random question - is the size only insufficient on the platforms where the bug was found? I'm wondering if we can skip the check on x8664 although maybe complicating things isn't worth that micro optimization.

@onlined
Copy link
Contributor Author

onlined commented Mar 10, 2022

From this answer, it seems like on Linux x86_64 and Linux arm64, the user space addresses (57 bit long maximum) don't reach PY_SSIZE_T_MAX (63 bit long on these platforms), so the bug doesn't cause any trouble. But on different processor architectures and kernels, this might change. I am not sure about going for that optimization, I think it's not worth relying on an implementation detail (if this is guaranteed, my opinion could change) for eliminating a function call and an integer comparison.

@jhance jhance merged commit 1a0a49a into python:master Mar 11, 2022
@mr-c
Copy link
Contributor

mr-c commented Mar 11, 2022

Thanks!

@onlined onlined deleted the mypyc-id-fix-overflow branch March 14, 2022 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mypyc 0.910 - 0.931 test failures on i686 (and armel, armhf)
3 participants