classification
Title: silent truncations in socket.htons and socket.ntohs
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Oren Milman, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2016-10-01 15:06 by Oren Milman, last changed 2017-08-17 08:14 by Oren Milman. 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) * (Python committer) 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) * (Python committer) 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
2017-08-17 08:14:54Oren Milmansettitle: Deprecated silent truncations in socket.htons and socket.ntohs. -> silent truncations in socket.htons and socket.ntohs
2017-08-17 08:13:59Oren Milmansettitle: keyword arguments -> Deprecated silent truncations in socket.htons and socket.ntohs.
2017-08-17 08:12:49Oren Milmansettitle: silent truncations in socket.htons and socket.ntohs -> keyword arguments
2017-03-31 16:36:19dstufftsetpull_requests: + pull_request929
2016-10-02 09:39:43serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg277872

stage: patch review -> resolved
2016-10-02 09:35:15python-devsetnosy: + python-dev
messages: + msg277871
2016-10-02 06:51:22Oren Milmansetfiles: + patchedCPythonTestOutput_ver2.txt
2016-10-02 06:50:46Oren Milmansetfiles: + issue28332_ver2.diff

messages: + msg277857
2016-10-01 19:40:14serhiy.storchakasetnosy: + serhiy.storchaka

messages: + msg277830
stage: patch review
2016-10-01 15:07:52Oren Milmansetfiles: + issue28332_ver1.diff
keywords: + patch
2016-10-01 15:07:08Oren Milmansetfiles: + patchedCPythonTestOutput_ver1.txt
2016-10-01 15:06:10Oren Milmancreate