Created on 2016-10-01 15:06 by Oren Milman, last changed 2022-04-11 14:58 by admin. This issue is now closed.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | |
| CPythonTestOutput.txt | Oren Milman, 2016-10-01 15:05 | test output of CPython without my patches (tested on my PC) | ||
| patchedCPythonTestOutput_ver1.txt | Oren Milman, 2016-10-01 15:07 | test output of CPython with my patches (tested on my PC) - ver1 | ||
| issue28332_ver1.diff | Oren Milman, 2016-10-01 15:07 | proposed patches diff file - ver1 | review | |
| issue28332_ver2.diff | Oren Milman, 2016-10-02 06:50 | proposed patches diff file - ver2 | review | |
| patchedCPythonTestOutput_ver2.txt | Oren Milman, 2016-10-02 06:51 | test output of CPython with my patches (tested on my PC) - ver2 | ||
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 552 | closed | dstufft, 2017-03-31 16:36 | |
| Messages (5) | |||
|---|---|---|---|
| msg277820 - (view) | Author: Oren Milman (Oren Milman) * | Date: 2016-10-01 15:05 | |
------------ current state ------------ Due to the implementation of socket_htons (in Modules/socketmodule.c), in case the received integer does not fit in 16-bit unsigned integer, but does fit in a positive C int, it is silently truncated to 16-bit unsigned integer (before converting to network byte order): >>> import socket >>> hex(socket.htons(0x1234)) '0x3412' >>> hex(socket.htons(0x81234)) '0x3412' >>> hex(socket.htons(0x881234)) '0x3412' >>> hex(socket.htons(0x8881234)) '0x3412' >>> hex(socket.htons(0x88881234)) Traceback (most recent call last): File "<stdin>", line 1, in <module> OverflowError: Python int too large to convert to C long >>> Likewise, socket.ntohs has the same silent truncation feature, due to the implementation of socket_ntohs. ISTM this silent truncation feature has the potential to conceal nasty bugs, and I guess it is rarely used in purpose. With regard to relevant changes made in the past: * The silent truncation was there since the two functions were first added, in changeset 3673 (https://hg.python.org/cpython/rev/f6ace61c3dfe). * A check whether the received integer is negative was added (to each of the two functions) in changeset 40632 (https://hg.python.org/cpython/rev/6efe3a4b10ac), as part of #1635058. Note the lack of discussion in #1635058 and #1619659 about backward compatibility. It might suggest that Guido didn't hesitate to make the change, even though at the time, the four conversion functions (socket.htons, socket.ntohs, socket.htonl and socket.ntohl) were already in the wild for 10 years. ------------ proposed changes ------------ 1. In Modules/socketmodule.c, raise a DeprecationWarning before silently truncating the received integer. In Python 3.8, replace the DeprecationWarning with an OverflowError. 2. In Lib/test/test_socket.py, add tests to verify a DeprecationWarning is raised as expected. 3. In Doc/library/socket.rst, add a description of the silent truncation feature, and declare it is deprecated. ------------ diff ------------ The proposed patches diff file is attached. (I wasn't sure you would approve deprecating a feature that was in the wild for so long, but I implemented it anyway, as it was quite simple.) ------------ tests ------------ I ran 'python_d.exe -m test -j3' (on my 64-bit Windows 10) with and without the patches, and got quite the same output. (That also means my new tests in test_socket passed.) The outputs of both runs are attached. |
|||
| msg277830 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * | Date: 2016-10-01 19:40 | |
Looks reasonable. ntohl() and htonl() already raise an exception if the argument exceeds 32 bit. Added comments on Rietveld. |
|||
| msg277857 - (view) | Author: Oren Milman (Oren Milman) * | Date: 2016-10-02 06:50 | |
Thanks for the review :) I changed some stuff, according to your comments (and replied to one comment in Rietveld). Version2 diff and test output are attached. |
|||
| msg277871 - (view) | Author: Roundup Robot (python-dev) | Date: 2016-10-02 09:35 | |
New changeset 3da460ca854b by Serhiy Storchaka in branch 'default': Issue #28332: Deprecated silent truncations in socket.htons and socket.ntohs. https://hg.python.org/cpython/rev/3da460ca854b |
|||
| msg277872 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * | Date: 2016-10-02 09:39 | |
Thank you for you contribution Oren. Rules for promotion unsinged integers to signed integers are not simple. I changed PyLong_FromLong() to PyLong_FromUnsignedLong() for the clarity. |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:58:37 | admin | set | github: 72519 |
| 2017-08-17 08:14:54 | Oren Milman | set | title: Deprecated silent truncations in socket.htons and socket.ntohs. -> silent truncations in socket.htons and socket.ntohs |
| 2017-08-17 08:13:59 | Oren Milman | set | title: keyword arguments -> Deprecated silent truncations in socket.htons and socket.ntohs. |
| 2017-08-17 08:12:49 | Oren Milman | set | title: silent truncations in socket.htons and socket.ntohs -> keyword arguments |
| 2017-03-31 16:36:19 | dstufft | set | pull_requests: + pull_request929 |
| 2016-10-02 09:39:43 | serhiy.storchaka | set | status: open -> closed resolution: fixed messages: + msg277872 stage: patch review -> resolved |
| 2016-10-02 09:35:15 | python-dev | set | nosy:
+ python-dev messages: + msg277871 |
| 2016-10-02 06:51:22 | Oren Milman | set | files: + patchedCPythonTestOutput_ver2.txt |
| 2016-10-02 06:50:46 | Oren Milman | set | files:
+ issue28332_ver2.diff messages: + msg277857 |
| 2016-10-01 19:40:14 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages:
+ msg277830 |
| 2016-10-01 15:07:52 | Oren Milman | set | files:
+ issue28332_ver1.diff keywords: + patch |
| 2016-10-01 15:07:08 | Oren Milman | set | files: + patchedCPythonTestOutput_ver1.txt |
| 2016-10-01 15:06:10 | Oren Milman | create | |