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

socket sendmsg(), recvmsg() methods #50809

Closed
synapse mannequin opened this issue Jul 24, 2009 · 55 comments
Closed

socket sendmsg(), recvmsg() methods #50809

synapse mannequin opened this issue Jul 24, 2009 · 55 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@synapse
Copy link
Mannequin

synapse mannequin commented Jul 24, 2009

BPO 6560
Nosy @brettcannon, @jcea, @ncoghlan, @pitrou, @jackdied, @giampaolo, @brianmay
Superseder
  • bpo-12958: test_socket failures on Mac OS X
  • Files
  • srmsg.patch: sendmsg()/recvmsg() patch for rev 74186
  • sendrecvmsgtest.py: my tester script
  • baikie-hwundram.diff: Different patch, with msg_name and scatter/gather
  • baikie-hwundram-v2.diff: New version w/ docs and tests (work in progress)
  • baikie-hwundram-v2.1.diff: Added test fix for non-IPv6 builds
  • baikie-hwundram-v3.diff: New version; added RFC 3542 tests
  • baikie-hwundram-v4.diff: New version; minor changes
  • v4-replace-existing-classes.diff: Optional: replace SocketTCPTest, etc. with the classes from the sendmsg patch
  • baikie-hwundram-v5.diff
  • v5-replace-existing-classes.diff
  • baikie-hwundram-v5-hg.diff
  • baikie-hwundram-v6.diff
  • baikie-hwundram-v6-for-2.x.diff
  • issue6560_ssl_fixes_v1.diff: Attempt 1 at resolving SSL related buildbot failures
  • pass_fds_osx.diff
  • 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 = 'https://github.com/ncoghlan'
    closed_at = <Date 2011-09-11.05:46:03.256>
    created_at = <Date 2009-07-24.08:30:17.314>
    labels = ['type-feature', 'library']
    title = 'socket sendmsg(), recvmsg() methods'
    updated_at = <Date 2012-04-30.13:53:07.207>
    user = 'https://bugs.python.org/synapse'

    bugs.python.org fields:

    activity = <Date 2012-04-30.13:53:07.207>
    actor = 'python-dev'
    assignee = 'ncoghlan'
    closed = True
    closed_date = <Date 2011-09-11.05:46:03.256>
    closer = 'ncoghlan'
    components = ['Library (Lib)']
    creation = <Date 2009-07-24.08:30:17.314>
    creator = 'synapse'
    dependencies = []
    files = ['14556', '14557', '15561', '16417', '16422', '17483', '17659', '17660', '19962', '19963', '22180', '22256', '22257', '23011', '23027']
    hgrepos = []
    issue_num = 6560
    keywords = ['patch']
    message_count = 55.0
    messages = ['90875', '90876', '90877', '96400', '99842', '99951', '99970', '99972', '99976', '100232', '100317', '100351', '106684', '106797', '106925', '107745', '107746', '123501', '136496', '136595', '136688', '136721', '136725', '137216', '137344', '137376', '137688', '137711', '137719', '142656', '142685', '142690', '142695', '142778', '142780', '142787', '142804', '142809', '142812', '142814', '142815', '142816', '142822', '142874', '142885', '142897', '142901', '142982', '143082', '143412', '143714', '143855', '143933', '143947', '159693']
    nosy_count = 17.0
    nosy_names = ['brett.cannon', 'jcea', 'exarkun', 'ncoghlan', 'janssen', 'pitrou', 'therve', 'jackdied', 'baikie', 'giampaolo.rodola', 'schmichael', 'synapse', 'wiml', 'neologix', 'rosslagerwall', 'python-dev', 'brian']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = '12958'
    type = 'enhancement'
    url = 'https://bugs.python.org/issue6560'
    versions = ['Python 3.3']

    @synapse
    Copy link
    Mannequin Author

    synapse mannequin commented Jul 24, 2009

    This is the rewritten-from-scratch implementation of the
    sendmsg()/recvmsg() methods.
    Any comments / suggestions / flames are very welcome. Currently it
    supports what I need
    and I'm only releasing it because I don't have much time to develop it
    further in the
    forseeable future (1-2 months). It is rewritten from scratch, using the
    python c-api
    documents. I've tried my best, but I wouldn't bet that it works
    error-free. I'd be glad
    if someone could give me a review on what I've might done wrong.

    The features that are missing:

    • using scatter/gather
    • using it with non-stream oriented sockets (doesn't support addresses
      /msg_name/)

    These should be very easy to implement. If no one takes up the task of
    implementing the
    missing features than I'll do it of course, you just have to wait a
    little while. Of
    course any errors present in the code right now will be fixed.

    Thanks

    Kalman Gergely

    @synapse synapse mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Jul 24, 2009
    @synapse
    Copy link
    Mannequin Author

    synapse mannequin commented Jul 24, 2009

    the tester application

    @therve
    Copy link
    Mannequin

    therve mannequin commented Jul 24, 2009

    This is a duplicate (although updated patch) from bug bpo-1194378. It would
    still need unit tests...

    @baikie
    Copy link
    Mannequin

    baikie mannequin commented Dec 14, 2009

    Hi, I'm afraid there may have been some duplication of effort
    here - I set about reworking Heiko Wundram's original patch
    (issue bpo-1194378) without knowing about this one. I don't have
    unit tests yet either, but I saw this and thought I'd better post
    what I had. It includes the items on Kalman's to-do list.

    This patch is based on Heiko's original code, but changes the
    interface, dropping the special processing of SCM_RIGHTS, etc. (I
    think the encoding/decoding would be better handled with separate
    functions/classes), and adds source address (msg_name) support
    and scatter/gather I/O via sendmsg() and a new recvmsg_into()
    method, as well as fixing various bugs and limitations.

    Comments/flames welcome too. You'll see a few XXX comments in
    the code. One in particular refers to the msg_name value from
    recvmsg() on a connected socket; I've said in the docstring that
    it is "unspecified" in this case, but it might or might not
    contain a valid address, depending on the OS. These methods may
    also need to be conditionally compiled if, say, CMSG_SPACE (or
    sendmsg/recvmsg?) isn't available somewhere.

    I didn't add a facility to receive into only part of a buffer in
    recvmsg_into(), like the one in recv_into(), since memoryview
    objects make it redundant AFAICT.

    @jackdied
    Copy link
    Contributor

    I'm +1 on adding it but it requires unit tests. Also, recvmsg/sendmsg appear to be *Nix only functions so someone with a windows box would need to test this to see if it blows up.

    @wiml
    Copy link
    Mannequin

    wiml mannequin commented Feb 23, 2010

    I just ran across yet another implementation of sendmsg support for python sockets, whose feature set seems to complement Kalman Gergely's implementation:
    http://www.pps.jussieu.fr/~ylg/PyXAPI by Yves Legrandgerard

    (Out of curiosity, people have been submitting requests for, and patches with implementations of, this feature for five or more years now and they're never accepted. Is there some opposition to sendmsg support in Python? It's a missing feature that bites me pretty regularly.)

    @jackdied
    Copy link
    Contributor

    I've been digging into the patch. Is there a reason sendmsg() wants an iterable of buffers instead of just accepting a str? The list-of-buffers more closely matches the underlying syscall but I'm not sure what the python benefit is, especially when recvmsg() only returns a single value (it only creates 1 iovec under the covers). Python doesn't have "readv" like methods so making sendmsg/recvmsg work like recv/send (straight strings) seems like the way to go.

    Also, the "y*" format character for packing/unpacking tuples is no longer supported - I'm assuming it used to mean buffers.

    Does anyone have a good reference for using recvmsg/sendmsg? I read the man pages and googled around but couldn't find anything. I have no experience with using the calls in-the-wild.

    @jackdied
    Copy link
    Contributor

    Additional data point: the perl version takes a single scalar (instead of a list of scalars) for use with sendmsg()

    http://search.cpan.org/~MJP/Socket-MsgHdr-0.01/MsgHdr.pm

    @jackdied
    Copy link
    Contributor

    one of the other sprinters just pointed out that Modules/_multiprocessing.c (py3k branch) uses sendmsg/recvmsg internally to pass file descriptors back and forth. The code is very short and readable.

    @baikie
    Copy link
    Mannequin

    baikie mannequin commented Mar 1, 2010

    Thanks for your interest! I'm actually still working on the
    patch I posted, docs and a test suite, and I'll post something
    soon.

    Yes, you could just use b"".join() with sendmsg() (and get
    slightly annoyed because it doesn't accept buffers ;) ). I made
    sendmsg() take multiple buffers because that's the way the system
    call works, but also to match recvmsg_into(), which gives you the
    convenience of being able to receive part of the message into a
    bytearray and part into an array.array("i"), say, if that's how
    the data is formatted.

    As you might know, gather-write with sendmsg() can give a
    performance benefit by letting the kernel assemble the message
    while copying the data from userspace rather than having
    userspace copy the data once to form the message and then having
    the kernel copy it again when the system call is made. I suppose
    with Python you just need a larger message to see the benefit :)
    Since it can read from buffers, though, socket.sendmsg() can pull
    a large chunk of data straight out of an mmap object, say, and
    attach headers from a bytes object without the mmapped data being
    touched by Python at all (or even entering userspace, in this
    case).

    The patch is for 3.x, BTW - "y*" is valid there (and does take a
    buffer).

    As for a good reference, I haven't personally seen one. There's
    POSIX and RFC 3542, but they don't provide a huge amount of
    detail. Perhaps the (updated) W. Richard Stevens networking
    books? I've got the Stevens/Rago second edition of Advanced
    Programming in the Unix Environment, which discusses FD and
    credential passing with sendmsg/recvmsg, but not very well (it
    misuses CMSG_LEN, for one thing). The networking books were
    updated by different people though, so perhaps they do better.

    The question of whether to use CMSG_NXTHDR() to step to the next
    header when constructing the buffer for sendmsg() is a bit murky,
    in particular. I've assumed that this is the way to do it since
    the examples in RFC 3542 (and most of the code I've seen
    generally) use CMSG_FIRSTHDR() to get the initial pointer, but
    I've found that glibc's CMSG_NXTHDR() can (wrongly, I think)
    return NULL if the buffer hasn't been zero-filled beforehand
    (this causes segfaults with the patch I initially posted).

    @wim:

    Yes, the rfc3542 module from that package looks as if it would be
    usable with these patches - although it's Python 2-only, GPL-only
    and looks unmaintained. Those kind of ancillary data
    constructors will actually be needed to make full portable use of
    sendmsg() and recvmsg() for things like IPv6, SCTP, Linux's
    socket error queues, etc. The same goes for data for the
    existing get/setsockopt() methods, in fact - the present
    suggestion to use the struct module is pretty inadequate when
    there are typedefs involved and implementations might add and
    reorder fields, etc.

    The objects in that package seem a bit overcomplicated, though,
    messing about with setter methods instead of just subclassing
    "bytes" and having different constructors to create the object
    from individual arguments or received bytes (say, ucred(1, 2, 3)
    or ucred.from_bytes(...)).

    Maybe the problem of testing patches well has been putting people
    off so far? Really exercising the system's CMSG_*HDR() macros in
    particular isn't entirely straightforward. I suppose there's
    also a reluctance to write tests while still uncertain about how
    to present the interface - that's another reason why I went for
    the most general multiple-buffer form of sendmsg()!

    @baikie
    Copy link
    Mannequin

    baikie mannequin commented Mar 2, 2010

    OK, here's a new version as a work in progress. A lot of the new
    stuff is uncommented (particularly the support code for the
    tests), but there are proper docs this time and a fairly complete
    test suite (but see below).

    There are a couple of changes to the interface (hopefully the
    last). The recvmsg() methods no longer receive ancillary data by
    default, since calling them on an AF_UNIX socket with the old
    default buffer could allow a malicious sender to send unwanted
    file descriptors up to receiver's resource limit, and in a
    multi-threaded program, another thread could then be prevented
    from opening new file descriptors before the receiving thread had
    a chance to close the unwanted ones.

    Since the ancillary buffer size argument is now more likely to
    need a value, I've moved it to second place; the basic argument
    order is now the same as in Kalman Gergely's patch. CMSG_LEN()
    and CMSG_SPACE() are now provided.

    I've also used socket.error instead of ValueError when rejecting
    some buffer object/array for being too big to handle, since the
    system call itself might cause socket.error to be raised for a
    smaller (oversized) object, failing with EMSGSIZE or whatever.

    The code is now much more paranoid about checking the results of
    the CMSG_*() macros, and will raise RuntimeError if it finds its
    assumptions are not met.

    I'd still like to add tests for receiving some of the RFC 3542
    ancillary data items, especially since the SCM_RIGHTS tests can't
    always (ever?) test recvmsg() with multiple items (if you send
    two FD arrays, the OS can coalesce them into a single array
    before delivering them).

    @baikie
    Copy link
    Mannequin

    baikie mannequin commented Mar 3, 2010

    I just found that the IPv6 tests don't get skipped when IPv6 is
    available but disabled in the build - you can create IPv6
    sockets, but not use them :/ This version fixes the problem.

    @baikie
    Copy link
    Mannequin

    baikie mannequin commented May 28, 2010

    Here is a new version of the patch; I've added some tests which
    use the RFC 3542 interface (IPv6 advanced API) and am now quite
    happy with it generally.

    As well as Linux, I've tested it on an old (unsupported) FreeBSD
    5.3 installation, which required a few changes in the tests and
    the code, but is probably representative of many socket
    implementations. Testing on other systems would be appreciated!

    The main issue was that when truncating ancillary data, FreeBSD
    seemed to just copy as much into the buffer as would fit and did
    not adjust the last item's cmsg_len member to reflect the amount
    of data that was actually present, so the item would appear to
    extend past the end of the buffer. The last version of the patch
    detected this and raised RuntimeError, which prevented any
    truncated receives from succeeding. The new version instead
    issues a warning and returns as much of the last item as is in
    the buffer.

    The warning could perhaps be disabled for systems like this,
    given that it happens every time ancillary data is truncated, but
    truncation generally shouldn't happen in a program's normal
    operation, and on other platforms a bad cmsg_len value might
    indicate that the returned data is actually incorrect in some
    way.

    After some investigation, I've stuck with using CMSG_FIRSTHDR()
    and CMSG_NXTHDR() to step through the headers when assembling the
    ancillary data in sendmsg().

    The KAME IPv6 userspace utilities at [1] include several programs
    which send multiple control messages at once, and these always
    use CMSG_NXTHDR() to advance to the next uninitialized header,
    while some (but not all) of them zero-fill the buffer beforehand,
    suggesting they ran into the issue with glibc's macros returning
    NULL (KAME developed the BSD IPv6 stack, and the zero-filling
    isn't necessary with the BSD macros).

    The alternative would be to add CMSG_SPACE(size) to the pointer
    to get to the next header. Going by the diagram in RFC 3542,
    that should be equivalent, but if some system defined
    CMSG_SPACE(len) as, say, CMSG_LEN(len) + 3, instead of
    (CMSG_LEN(len) + 3) & ~3, it would probably go unnoticed until
    someone tried to use CMSG_SPACE() that way. So given the KAME
    example, I think using CMSG_NXTHDR() with a zero-filled buffer is
    the way to go.

    [1] http://www.kame.net/dev/cvsweb2.cgi/kame/kame/kame/

    @pitrou
    Copy link
    Member

    pitrou commented May 31, 2010

    Would you like to upload your patch to http://codereview.appspot.com/? It would make reviewing easier.

    @baikie
    Copy link
    Mannequin

    baikie mannequin commented Jun 2, 2010

    OK. I don't like creating/using a Google account, but here it is:

    http://codereview.appspot.com/1487041/show

    @baikie
    Copy link
    Mannequin

    baikie mannequin commented Jun 13, 2010

    New version with minor changes. Will also upload at http://codereview.appspot.com/1487041/show

    @baikie
    Copy link
    Mannequin

    baikie mannequin commented Jun 13, 2010

    Optional patch to replace SocketTCPTest, etc. with the classes from the sendmsg patch.

    @baikie
    Copy link
    Mannequin

    baikie mannequin commented Dec 6, 2010

    Updated the patch to use the new BEGIN/END_SELECT_LOOP macros.

    Also fixed a bug I spotted while doing this whereby when a
    timeout was set, errors in select() or poll() would be
    misreported as timeouts.

    Will also upload at http://codereview.appspot.com/1487041/show

    @brianmay
    Copy link
    Mannequin

    brianmay mannequin commented May 22, 2011

    What needs to happen to get recvmsg() supported in Python?

    recvmsg() is required to get get transparent UDP proxies working under Linux using tproxy, the code needs to run recvmsg() to be able to find out what the original destination address was for the the packet.

    Thanks

    Brian May

    @synapse
    Copy link
    Mannequin Author

    synapse mannequin commented May 23, 2011

    On 05/22/11 03:14, Brian May wrote:

    Brian May<brian@microcomaustralia.com.au> added the comment:

    What needs to happen to get recvmsg() supported in Python?

    recvmsg() is required to get get transparent UDP proxies working under Linux using tproxy, the code needs to run recvmsg() to be able to find out what the original destination address was for the the packet.

    Thanks

    Brian May

    ----------
    nosy: +brian


    Python tracker<report@bugs.python.org>
    <http://bugs.python.org/issue6560\>


    Hello Brian!

    It's been a while I had a look at that code. As far as I remember though
    the code is fairly decent not
    taking the missing unit tests into account. There are a few todos, and
    also a pretty bad bug that I've fixed
    but not committed. The TODOs include better parsing of auxiliary data,
    support for scatter-gather, addressed
    messages. If you wish I can send you the latest patch that has the bug
    fixed and applies to 3.2. Do you want
    this to be pushed to 3.3?

    Gergely Kalman

    @baikie
    Copy link
    Mannequin

    baikie mannequin commented May 23, 2011

    On Mon 23 May 2011, Gergely Kálmán wrote:

    It's been a while I had a look at that code. As far as I remember though
    the code is fairly decent not
    taking the missing unit tests into account. There are a few todos, and
    also a pretty bad bug that I've fixed
    but not committed. The TODOs include better parsing of auxiliary data,
    support for scatter-gather, addressed
    messages. If you wish I can send you the latest patch that has the bug
    fixed and applies to 3.2.

    Erm, have you seen the separately-implemented patch I posted at
    http://bugs.python.org/file19962/baikie-hwundram-v5.diff ? It's
    basically complete IIRC.

    @brianmay
    Copy link
    Mannequin

    brianmay mannequin commented May 24, 2011

    Hello,

    Are there any problems applying the v5 version of the patch to 3.3?

    Also is there any remote chance for a backport to 2.7?

    Thanks

    @synapse
    Copy link
    Mannequin Author

    synapse mannequin commented May 24, 2011

    No, indeed this is a lot better.

    @baikie
    Copy link
    Mannequin

    baikie mannequin commented May 29, 2011

    On Tue 24 May 2011, Brian May wrote:

    Are there any problems applying the v5 version of the patch to 3.3?

    Well, it still works for me, apart from a trivial patch conflict:
    I'm attaching a fresh diff (with no changes) against a recent hg
    revision.

    Antoine did previously question the necessity of all the various
    "abstract" mixin classes I used to form unit test fixtures for
    different socket types, but the most recent version does, for
    instance, use UDPTestBase and TCPTestBase both with and without
    the threading and connection-establishing mixins, respectively
    (and the replace-existing-classes patch can still be applied to
    remove the redundancy with SocketTCPTest, etc.).

    Apart from that, I don't know. Perhaps a review by someone
    familiar with the interface would help?

    Also is there any remote chance for a backport to 2.7?

    I'd be happy to do one, but I'm pretty sure python.org's 2.x line
    is closed to new features. Perhaps some fork of CPython might be
    willing to accept it, though - I don't know.

    @brianmay
    Copy link
    Mannequin

    brianmay mannequin commented May 31, 2011

    Have tested my code with this patch, the recvmsg(...) call seems to work fine.

    Also had a half-hearted attempt at porting to Python 2.7, but didn't get past compiling, the code requires BEGIN_SELECT_LOOP and END_SELECT_LOOP macros that aren't defined in Python 2.7 - I tried copying the definitions from Python 3.3, but that didn't work either. Not sure if it is worth the effort if Python 2.7 is closed to new features.

    Brian May

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented May 31, 2011

    What needs to happen to get recvmsg() supported in Python?

    Well, I guess that the only reason is that no committer is motivated
    enough to bring this into Python: it's a rather large patch, and
    honestly, I'm not sure that many people are going to use it.
    The feature I personally like the most about sendmsg/recvmsg is the
    ability to do scatter-gather I/O, but if the performance is critical,
    then I won't be using Python.
    I know that sendmsg also has some other advantages (passing FDs,
    ancillary data...).

    recvmsg() is required to get get transparent UDP proxies working under Linux
    using tproxy, the code needs to run recvmsg() to be able to find out what the
    original destination address was for the the packet.

    Sounds like a job for raw sockets, no? (well, you need CAP_NET_RAW)

    In short, I think that you just need to find a core developer
    interested, I personally am not (but I'm not opposed to it either :-).

    @pitrou
    Copy link
    Member

    pitrou commented Jun 5, 2011

    > What needs to happen to get recvmsg() supported in Python?

    Well, I guess that the only reason is that no committer is motivated
    enough to bring this into Python: it's a rather large patch, and
    honestly, I'm not sure that many people are going to use it.
    The feature I personally like the most about sendmsg/recvmsg is the
    ability to do scatter-gather I/O, but if the performance is critical,
    then I won't be using Python.
    I know that sendmsg also has some other advantages (passing FDs,
    ancillary data...).

    Modules/_multiprocessing already has code using sendmsg/recvmsg,
    precisely to pass FDs IIRC.
    Generic support for sendmsg() and recvmsg() in the socket module would
    allow to rewrite that code in pure Python.

    @baikie
    Copy link
    Mannequin

    baikie mannequin commented Jun 5, 2011

    On Tue 31 May 2011, Brian May wrote:

    Also had a half-hearted attempt at porting to Python 2.7, but
    didn't get past compiling, the code requires BEGIN_SELECT_LOOP
    and END_SELECT_LOOP macros that aren't defined in Python 2.7 -
    I tried copying the definitions from Python 3.3, but that
    didn't work either. Not sure if it is worth the effort if
    Python 2.7 is closed to new features.

    Well, if it's any use to anyone, here is a backport to 2.x (the
    *_SELECT_LOOP macros aren't important, and it's otherwise
    straightforward).

    Also attaching a new 3.x patch which fixes some deprecated markup
    in the docs and adds the send/recvmsg() methods to
    test_wrapped_unconnected() in test_ssl.py.

    @ncoghlan ncoghlan reopened this Aug 23, 2011
    @ncoghlan ncoghlan self-assigned this Aug 23, 2011
    @ncoghlan
    Copy link
    Contributor

    Attached patch (ssl_fixes_v1) makes the presence of the sendmsg/recvmsg methods in the ssl module conditional on their being present in the underlying socket module.

    However, in doing this, I noticed that these methods will, at best, work during the time between connection and the socket going secure and were not added to the list of methods that the SSL is documented as exposing. Perhaps we should just ditch them entirely?

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Aug 23, 2011

    However, in doing this, I noticed that these methods will, at best, work during the time between connection and the socket going secure and were not added to the list of methods that the SSL is documented as exposing. Perhaps we should just ditch them entirely?

    +1

    @pitrou
    Copy link
    Member

    pitrou commented Aug 23, 2011

    However, in doing this, I noticed that these methods will, at best,
    work during the time between connection and the socket going secure
    and were not added to the list of methods that the SSL is documented
    as exposing. Perhaps we should just ditch them entirely?

    Well, apparently the SSLSocket object is meant to be usable in clear
    text mode. I'm not sure why it's like that, but your patch looks ok to
    me (except that it has a debug print() call).

    @ncoghlan
    Copy link
    Contributor

    That's the part I'm questioning though. I'm not clear why you'd ever do that instead of doing everything on the original socket before invoking ssl.wrap_socket.

    What I missed on the original patch before committing it (mea culpa) is that the SSL part is neither documented nor tested properly (the tests only check that it is disallowed on a secured SSLSocket, not that it works on a connected-but-not-secured-yet SSLSocket object).

    The absence of proper tests and documentation is the main reason I'm tempted to just revert those parts of the patch that touch the ssl module and its tests.

    @pitrou
    Copy link
    Member

    pitrou commented Aug 23, 2011

    That's the part I'm questioning though. I'm not clear why you'd ever do
    that instead of doing everything on the original socket before invoking
    ssl.wrap_socket.

    What I missed on the original patch before committing it (mea culpa) is
    that the SSL part is neither documented nor tested properly (the tests
    only check that it is disallowed on a secured SSLSocket, not that it
    works on a connected-but-not-secured-yet SSLSocket object).

    Bill, do you know?

    The absence of proper tests and documentation is the main reason I'm tempted
    to just revert those parts of the patch that touch the ssl module and its
    tests.

    Then perhaps raise NotImplementedError, so that people know it's deliberate and not an oversight.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 23, 2011

    New changeset fd10d042b41d by Nick Coghlan in branch 'default':
    Remove the SSLSocket versions of sendmsg/recvmsg due to lack of proper tests and documentation in conjunction with lack of any known use cases (see issue bpo-6560 for details)
    http://hg.python.org/cpython/rev/fd10d042b41d

    @ncoghlan
    Copy link
    Contributor

    As you can see, I just pushed a change that removed the new methods from SSLSocket objects. If anyone wants to step up with a valid use case (not already covered by wrap_socket), preferably with a patch to add them back that includes proper tests and documentation changes, please open a new feature request and attach the new patch to that issue.

    @ncoghlan
    Copy link
    Contributor

    Regarding the 'missing methods' aspect, the SSL docs are already pretty clear that SSLSocket objects don't expose the full socket API:

    http://docs.python.org/dev/library/ssl#ssl-sockets

    Those docs are actually what really got me started down the path of wondering whether or not these new methods even belong on SSLSocket in the first place.

    @ncoghlan
    Copy link
    Contributor

    And the Windows buildbots are now happy (at least with respect to this change, anyway - they're still griping about a few other issues).

    I don't know if it's feasible to support these new APIs at the socket module level on Windows, but any patches along those lines should also be placed in a new issue rather than being added to this one.

    @pitrou
    Copy link
    Member

    pitrou commented Aug 24, 2011

    The OS X buildbots show some failures:
    http://www.python.org/dev/buildbot/all/builders/AMD64%20Snow%20Leopard%202%203.x

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Aug 24, 2011

    Here's a patch skipping testFDPassSeparate and
    testFDPassSeparateMinSpace on OS X < 10.5, due to known kernel bugs
    (see http://developer.apple.com/library/mac/#qa/qa1541/_index.html).
    For InterruptedSendTimeoutTest and testInterruptedSendmsgTimeout, it
    also looks like a kernel bug.
    There could be another explanation, though: if, for some reason, other
    threads are running at that time, the signal might be delivered to
    another thread, and our main thread remains stuck on sendto/sendmsg
    once the socket buffer is full. I'm however not sure why this would
    only affect OS X (since FreeBSD behaves in the same way when it comes
    to signals, contrarily to Linux). Also, I'm not sure why this would
    not affect recv/recvmsg.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Aug 24, 2011

    As noted by Antoine, the OS X 10.5 buildbots are also failing.

    @baikie
    Copy link
    Mannequin

    baikie mannequin commented Aug 24, 2011

    On Tue 23 Aug 2011, Nick Coghlan wrote:

    As you can see, I just pushed a change that removed the new
    methods from SSLSocket objects. If anyone wants to step up with
    a valid use case (not already covered by wrap_socket),
    preferably with a patch to add them back that includes proper
    tests and documentation changes, please open a new feature
    request and attach the new patch to that issue.

    Hi, sorry about the trouble caused by the broken tests, but
    SSLSocket should at least override sendmsg() to stop misguided
    programs sending data in the clear:

    http://bugs.python.org/issue12835

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Aug 25, 2011

    The OS X buildbots show some failures:

    It seems to fail consistently on every OS X version.
    I've had another look both at the code and the test, and couldn't find anything wrong with it.
    Since there are a number of known bugs pertaining to FD passing on OS X - even on recent versions - I'd suggest to skip those tests on this platform.
    As for sendto/sendmsg not being interrupted by the signal, I'd be curious to see if running test_socket alone solves the problem (just to make sure no other thread is running, which might receive the signal).

    @ncoghlan
    Copy link
    Contributor

    Putting this back to open until we decide what to do about the OS X test failures. It sounds like it could really do with some more poking and prodding to figure out whether or not it poses a potential security risk or is just a relatively cosmetic problem with the API, so I'm reluctant to just skip the failing tests at this point.

    @ncoghlan ncoghlan reopened this Aug 27, 2011
    @janssen
    Copy link
    Mannequin

    janssen mannequin commented Sep 2, 2011

    I'll take a look at this next week, when I'm more on-line again.

    Bill

    2011/8/25 Charles-François Natali <report@bugs.python.org>:

    Charles-François Natali <neologix@free.fr> added the comment:

    > The OS X buildbots show some failures:

    It seems to fail consistently on every OS X version.
    I've had another look both at the code and the test, and couldn't find anything wrong with it.
    Since there are a number of known bugs pertaining to FD passing on OS X - even on recent versions - I'd suggest to skip those tests on this platform.
    As for sendto/sendmsg not being interrupted by the signal, I'd be curious to see if running test_socket alone solves the problem (just to make sure no other thread is running, which might receive the signal).

    ----------


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue6560\>


    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Sep 7, 2011

    If Bill gets a chance to investigate this before the weekend, great, otherwise my plan to stop making noise in the buildbot results will be to:

    1. Create a separate issue specifically for the errors reported by the Mac OS X buildbots (allowing the problem to be spelled out more clearly for readers, and also allowing the feature request itself to be closed)

    2. Flag the offending tests as expected failures on Mac OS X, with a pointer back to the new tracker issue.

    That way, if these failures are due to underlying OS bugs or limitations (as they appear to be), we'll get a clear indication in the buildbots when Apple have fixed the relevant problems.

    @ncoghlan
    Copy link
    Contributor

    Closing the feature request as complete. The remaining Mac OS X buildbot issues now have their own tracker item: bpo-12958

    @janssen
    Copy link
    Mannequin

    janssen mannequin commented Sep 12, 2011

    I'm guessing these things are due to interaction with some Apple
    security update, as the buildbots were working well 8 months ago.

    Bill

    On Wed, Sep 7, 2011 at 4:01 PM, Nick Coghlan <report@bugs.python.org> wrote:

    Nick Coghlan <ncoghlan@gmail.com> added the comment:

    If Bill gets a chance to investigate this before the weekend, great, otherwise my plan to stop making noise in the buildbot results will be to:

    1. Create a separate issue specifically for the errors reported by the Mac OS X buildbots (allowing the problem to be spelled out more clearly for readers, and also allowing the feature request itself to be closed)

    2. Flag the offending tests as expected failures on Mac OS X, with a pointer back to the new tracker issue.

    That way, if these failures are due to underlying OS bugs or limitations (as they appear to be), we'll get a clear indication in the buildbots when Apple have fixed the relevant problems.

    ----------


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue6560\>


    @ncoghlan
    Copy link
    Contributor

    The feature patch for sendmsg/recvmsg support came with a swathe of new tests, and the failures are in those new tests rather than anything breaking in the old ones.

    As Charles-François noted though, it doesn't look like the feature implementation itself is doing anything wrong, just that there are limits to what Mac OS X allows us to do with it (hence why I closed this feature request and opened issue bpo-12958 to cover the task of updating the test suite to accurately reflect that situation).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 30, 2012

    New changeset e64bec91ac91 by Richard Oudkerk in branch 'default':
    Issue bpo-14669: Skip multiprocessing connection pickling test on MacOSX
    http://hg.python.org/cpython/rev/e64bec91ac91

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants