This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: _xxsubinterpreters: channel_send() may truncate ints on 32-bit platforms
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: izbyshev Nosy List: eric.snow, izbyshev, vstinner
Priority: normal Keywords: patch

Created on 2019-02-11 21:52 by izbyshev, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 11822 merged izbyshev, 2019-02-11 21:57
Messages (10)
msg335272 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2019-02-11 21:52
Victor Stinner pointed out that on x86 Gentoo Installed with X 3.x buildbot, there is a compiler warning:

Python/pystate.c:1483:18: warning: cast to pointer from integer of
different size [-Wint-to-pointer-cast]

(https://buildbot.python.org/all/#/builders/103/builds/2067)

This warning reveals a bug:

static int
_long_shared(PyObject *obj, _PyCrossInterpreterData *data)
{
    int64_t value = PyLong_AsLongLong(obj);
    if (value == -1 && PyErr_Occurred()) {
        if (PyErr_ExceptionMatches(PyExc_OverflowError)) {
            PyErr_SetString(PyExc_OverflowError, "try sending as bytes");
        }
        return -1;
    }
    data->data = (void *)value;

A 64-bit value is converted to void *, which is 32-bit on 32-bit platforms.

I've implemented a PR that uses Py_ssize_t instead to match the pointer size and to preserve the ability to work with negative numbers.
msg335281 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-02-12 01:37
Is "long long" (AKA int64_t) 32 bits or 64 bits on a 32-bit platform?  It it's always 32 bits then there is no problem here.  Otherwise I agree we have a problem to fix.  My understanding is that it is the former (always 32 bits).
msg335291 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2019-02-12 09:44
"long long" is mandated to be at least 64-bit by C99 (5.2.4.2.1 Sizes of integer types). If it were 32-bit, no warnings would have been issued.
msg335299 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-02-12 12:55
What matters here is that _PyCrossInterpreterData.data type is void*. So the largest integer that you can store is intptr_t (signed) or uintptr_t (unsigned). In practice, sizeof(void*) == sizeof(intptr_t) == sizeof(uintptr_t) == sizeof(size_t) == sizeof(ssize_t). IMHO using size_t or ssize_t is a good solution, since it's well supported in Python.
msg335308 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-02-12 15:40
Having slept on it and given the extra info you've provided I'm more comfortable with the proposed solution now. :)

So the result will be that on 32-bit architectures only 32-bit (signed) ints will be allowed through channels.  On 64-bit platforms (and higher) it will still be 64-bit ints.  That's a livable outcome, particularly since 32-bit platforms already have to deal with such variation already. :)  Plus, the consistency with sys.maxsize makes sense.

The alternative (on 32-bit arch) would be to send through a struct containing the long long, similar to what we do for str and bytes.  I don't think that's worth it.  If it matters we can do something like that later (since it's an implementation detail).

Consequently, I've approved the PR.  I'll merge it in a little while.

Thanks for working on this.
msg335309 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-02-12 15:42
FTR, issue #34569 covered similar ground.
msg335317 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-02-12 16:06
New changeset 16f842da3c862d76a1177bd8ef9579703c24fa5a by Eric Snow (Alexey Izbyshev) in branch 'master':
bpo-35972: _xxsubinterpreters: Fix potential integer truncation on 32-bit in channel_send() (gh-11822)
https://github.com/python/cpython/commit/16f842da3c862d76a1177bd8ef9579703c24fa5a
msg335318 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-02-12 16:07
Thanks, Alexey!
msg335410 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-02-13 11:16
> New changeset 16f842da3c862d76a1177bd8ef9579703c24fa5a by Eric Snow (Alexey Izbyshev) in branch 'master':
> bpo-35972: _xxsubinterpreters: Fix potential integer truncation on 32-bit in channel_send() (gh-11822)

It seems like this change introduced a reference leak: bpo-35984.
msg335459 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-02-13 15:51
ack
History
Date User Action Args
2022-04-11 14:59:11adminsetgithub: 80153
2019-02-13 15:51:52eric.snowsetmessages: + msg335459
2019-02-13 11:16:05vstinnersetmessages: + msg335410
2019-02-12 16:07:30eric.snowsetstatus: open -> closed
resolution: fixed
messages: + msg335318

stage: commit review -> resolved
2019-02-12 16:06:47eric.snowsetmessages: + msg335317
2019-02-12 15:42:16eric.snowsetstatus: pending -> open

messages: + msg335309
2019-02-12 15:41:06eric.snowsetstatus: open -> pending
components: + Interpreter Core, - Extension Modules
stage: patch review -> commit review
2019-02-12 15:40:19eric.snowsetmessages: + msg335308
2019-02-12 12:55:24vstinnersetmessages: + msg335299
2019-02-12 09:44:54izbyshevsetmessages: + msg335291
2019-02-12 01:37:23eric.snowsetmessages: + msg335281
2019-02-11 21:57:07izbyshevsetkeywords: + patch
stage: patch review
pull_requests: + pull_request11852
2019-02-11 21:52:48izbyshevcreate