Issue 35972: _xxsubinterpreters: channel_send() may truncate ints on 32-bit platforms (original) (raw)

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🔞 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:11 admin set github: 80153
2019-02-13 15:51:52 eric.snow set messages: +
2019-02-13 11:16:05 vstinner set messages: +
2019-02-12 16:07:30 eric.snow set status: open -> closedresolution: fixedmessages: + stage: commit review -> resolved
2019-02-12 16:06:47 eric.snow set messages: +
2019-02-12 15:42:16 eric.snow set status: pending -> openmessages: +
2019-02-12 15:41:06 eric.snow set status: open -> pendingcomponents: + Interpreter Core, - Extension Modulesstage: patch review -> commit review
2019-02-12 15:40:19 eric.snow set messages: +
2019-02-12 12:55:24 vstinner set messages: +
2019-02-12 09:44:54 izbyshev set messages: +
2019-02-12 01:37:23 eric.snow set messages: +
2019-02-11 21:57:07 izbyshev set keywords: + patchstage: patch reviewpull_requests: + <pull%5Frequest11852>
2019-02-11 21:52:48 izbyshev create