| msg335272 - (view) |
Author: Alexey Izbyshev (izbyshev) *  |
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) *  |
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) * |
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) * |
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) * |
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) * |
Date: 2019-02-12 15:42 |
| FTR, issue #34569 covered similar ground. |
|
|
| msg335317 - (view) |
Author: Eric Snow (eric.snow) * |
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) * |
Date: 2019-02-12 16:07 |
| Thanks, Alexey! |
|
|
| msg335410 - (view) |
Author: STINNER Victor (vstinner) * |
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) * |
Date: 2019-02-13 15:51 |
| ack |
|
|