Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

silent truncations in socket.htons and socket.ntohs #72519

Closed
orenmn mannequin opened this issue Oct 1, 2016 · 5 comments
Closed

silent truncations in socket.htons and socket.ntohs #72519

orenmn mannequin opened this issue Oct 1, 2016 · 5 comments
Labels
3.7 stdlib type-bug

Comments

@orenmn
Copy link
Mannequin

@orenmn orenmn mannequin commented Oct 1, 2016

BPO 28332
Nosy @serhiy-storchaka, @orenmn
PRs
  • #552
  • Files
  • CPythonTestOutput.txt: test output of CPython without my patches (tested on my PC)
  • patchedCPythonTestOutput_ver1.txt: test output of CPython with my patches (tested on my PC) - ver1
  • issue28332_ver1.diff: proposed patches diff file - ver1
  • issue28332_ver2.diff: proposed patches diff file - ver2
  • patchedCPythonTestOutput_ver2.txt: test output of CPython with my patches (tested on my PC) - ver2
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2016-10-02.09:39:43.267>
    created_at = <Date 2016-10-01.15:06:10.076>
    labels = ['3.7', 'type-bug', 'library']
    title = 'silent truncations in socket.htons and socket.ntohs'
    updated_at = <Date 2017-08-17.08:14:54.631>
    user = 'https://github.com/orenmn'

    bugs.python.org fields:

    activity = <Date 2017-08-17.08:14:54.631>
    actor = 'Oren Milman'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-10-02.09:39:43.267>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2016-10-01.15:06:10.076>
    creator = 'Oren Milman'
    dependencies = []
    files = ['44911', '44912', '44913', '44922', '44923']
    hgrepos = []
    issue_num = 28332
    keywords = ['patch']
    message_count = 5.0
    messages = ['277820', '277830', '277857', '277871', '277872']
    nosy_count = 3.0
    nosy_names = ['python-dev', 'serhiy.storchaka', 'Oren Milman']
    pr_nums = ['552']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue28332'
    versions = ['Python 3.7']

    @orenmn
    Copy link
    Mannequin Author

    @orenmn orenmn mannequin commented Oct 1, 2016

    ------------ 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 bpo-1635058.
    Note the lack of discussion in bpo-1635058 and bpo-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](https://github.com/python/cpython/blob/main/Lib/test/test_socket.py), add tests to verify a DeprecationWarning is raised as expected.
    
    3. In [Doc/library/socket.rst](https://github.com/python/cpython/blob/main/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.

    @orenmn orenmn mannequin added 3.7 stdlib type-bug labels Oct 1, 2016
    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Oct 1, 2016

    Looks reasonable. ntohl() and htonl() already raise an exception if the argument exceeds 32 bit. Added comments on Rietveld.

    @orenmn
    Copy link
    Mannequin Author

    @orenmn orenmn mannequin commented Oct 2, 2016

    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.

    @python-dev
    Copy link
    Mannequin

    @python-dev python-dev mannequin commented Oct 2, 2016

    New changeset 3da460ca854b by Serhiy Storchaka in branch 'default':
    Issue bpo-28332: Deprecated silent truncations in socket.htons and socket.ntohs.
    https://hg.python.org/cpython/rev/3da460ca854b

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Oct 2, 2016

    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.

    @orenmn orenmn mannequin changed the title silent truncations in socket.htons and socket.ntohs keyword arguments Aug 17, 2017
    @orenmn orenmn mannequin changed the title keyword arguments Deprecated silent truncations in socket.htons and socket.ntohs. Aug 17, 2017
    @orenmn orenmn mannequin changed the title Deprecated silent truncations in socket.htons and socket.ntohs. silent truncations in socket.htons and socket.ntohs Aug 17, 2017
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 stdlib type-bug
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant