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

asyncore's accept() is broken #50955

Closed
giampaolo opened this issue Aug 14, 2009 · 16 comments
Closed

asyncore's accept() is broken #50955

giampaolo opened this issue Aug 14, 2009 · 16 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-security A security issue

Comments

@giampaolo
Copy link
Contributor

giampaolo commented Aug 14, 2009

BPO 6706
Nosy @warsaw, @josiahcarlson, @pitrou, @giampaolo, @matejcik, @davidmalcolm
Files
  • accept.patch
  • accept.patch
  • accept.patch: adds handled_accepted() method
  • 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/giampaolo'
    closed_at = <Date 2010-11-01.15:19:36.489>
    created_at = <Date 2009-08-14.23:03:35.884>
    labels = ['type-security', 'library']
    title = "asyncore's accept() is broken"
    updated_at = <Date 2010-12-07.15:22:09.741>
    user = 'https://github.com/giampaolo'

    bugs.python.org fields:

    activity = <Date 2010-12-07.15:22:09.741>
    actor = 'barry'
    assignee = 'giampaolo.rodola'
    closed = True
    closed_date = <Date 2010-11-01.15:19:36.489>
    closer = 'giampaolo.rodola'
    components = ['Library (Lib)']
    creation = <Date 2009-08-14.23:03:35.884>
    creator = 'giampaolo.rodola'
    dependencies = []
    files = ['19001', '19005', '19104']
    hgrepos = []
    issue_num = 6706
    keywords = ['patch']
    message_count = 16.0
    messages = ['91579', '112315', '112699', '117240', '117329', '117356', '117357', '117358', '117359', '117360', '117873', '117925', '117981', '120003', '120132', '123556']
    nosy_count = 9.0
    nosy_names = ['barry', 'josiahcarlson', 'pitrou', 'giampaolo.rodola', 'matejcik', 'Arfrever', 'dmalcolm', 'santoso.wijaya', 'BreamoreBoy']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'security'
    url = 'https://bugs.python.org/issue6706'
    versions = ['Python 3.1', 'Python 2.7', 'Python 3.2']

    @giampaolo
    Copy link
    Contributor Author

    giampaolo commented Aug 14, 2009

    An old bad design choice in asyncore is how it forces the user to
    override handle_accept() and then call self.accept() to obtain a socket
    pair.

        def handle_accept(self):
            conn, addr = self.accept()

    The documentation itself shows the code above as an example of how an
    asyncore-based server should handle an incoming connection.
    What the doc doesn't say is that the user calling self.accept() is
    exposed to different risks:

    The right thing to do when one of such events occur is to "return" as no
    connection has really been established between client and server.

    Now, altough handling these kind of conditions is quite easy, the API
    design remains fundamentally wrong, as it forces the user to call
    accept().
    As asyncore.py is structured right now the only chance the user has to
    properly accepting a connection is manually handling all these cases in
    his subclass.

    My patch in attachment tries to solve this problem by defining a new
    "handle_accept_event()" method which takes care of all the error
    conditions described above resulting in handle_accept() be called *only
    when a connection really took place*.
    When the connection is established handle_accept_event() stores the
    socket pair as an attribute and the next call to accept() returns that
    pair.
    This preserves the current implementation without breaking any code as
    the user will have to override handle_accept() and then call accept() in
    the same manner [1], with the difference that self.accept() will always
    return a valid socket pair.

    [1]
    def handle_accept(self):
    conn, addr = self.accept()

    @giampaolo giampaolo added the stdlib Python modules in the Lib dir label Aug 14, 2009
    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Aug 1, 2010

    @giampaolo it looks as if you meant to attach a patch but didn't! :) If you do attach one could you also supply a modified unit test file with it, thanks.

    @BreamoreBoy BreamoreBoy mannequin added the type-bug An unexpected behavior, bug, or error label Aug 1, 2010
    @giampaolo
    Copy link
    Contributor Author

    giampaolo commented Aug 3, 2010

    Shame on me, it seems I totally forgot to attach the patch.
    Unfortunately the patch went lost but I'm going to try to rewrite it.
    As for tests, ECONN and EAGAIN error conditions are hardly reproducible unless you're using nmap.
    I'm assigning this to me for now as a reminder and update the issue as long as I have a working patch.

    @giampaolo giampaolo self-assigned this Aug 3, 2010
    @davidmalcolm
    Copy link
    Member

    davidmalcolm commented Sep 23, 2010

    giampaolo: did you ever rewrite the patch?

    For reference to other users:
    http://code.google.com/p/pyftpdlib/source/browse/trunk/pyftpdlib/ftpserver.py

    Note the complexity of the two handle_accept implementations in that file; both of them begin:
    try:
    sock, addr = self.accept()
    except TypeError:
    # sometimes accept() might return None (see issue 91)
    return
    except socket.error, err:
    # ECONNABORTED might be thrown on *BSD (see issue 105)
    if err[0] != errno.ECONNABORTED:
    logerror(traceback.format_exc())
    return
    else:
    # sometimes addr == None instead of (ip, port) (see issue 104)
    if addr == None:
    return

    @giampaolo
    Copy link
    Contributor Author

    giampaolo commented Sep 24, 2010

    Here's a rewriting attempt (not tested).
    Now that I look at it I must say it's quite ugly, so I don't think we should follow this road.
    An alternative I see is to return None in case of errors occurring on accept() and make this very clear in doc.
    Other than accept(), doc should explicitly show how to use handle_accept() in general, which would end up looking like this:

    class SomeServer(asyncore.dispatcher):
        
        ...
    
        def handle_accept():
           conn = self.accept()
           if conn is None:
               return
           else:
               sock, addr = conn
           ...
    ...
    

    A completely different approach could be to provide a new "TCPServer" class which deprecates direct accept() method usage. Something like this:

    class TCPServer(asyncore.dispatcher):
    
        def __init__(self, ip, port, handler, interface='', map=None):
            asyncore.dispatcher.__init__(self, map=map)
            self.create_socket(family=socket.AF_INET, type=socket.SOCK_STREAM)
            self.bind((ip, port))
            self.listen(5)
            self.handler = handler
    
        def handle_accept(self):
            try:
                sock, addr = self.accept()
            except TypeError:
                return
            except socket.error, err:
                if err[0] != errno.ECONNABORTED:
                    raise
                return
            else:
                if addr == None:
                    return
            handler = self.handler(conn, self._map)
    
        def writable(self):
            return 0

    ...but for some reason I don't like it either. Point is asyncore API design is fundamentally wrong and there's nothing we can do about it unless we break backward compatibility or a brand new "asyncore2" module is written.

    @giampaolo
    Copy link
    Contributor Author

    giampaolo commented Sep 25, 2010

    Patch in attachment makes accept() return None in case no connection takes place and modifies the doc to make this very clear, also providing an example on how an asyncore-based server should be properly set-up .

    @pitrou
    Copy link
    Member

    pitrou commented Sep 25, 2010

    EAGAIN can be raised too. I never experienced this error condition
    myself in pyftpdlib

    From the accept() man page:

    EAGAIN or EWOULDBLOCK
    The socket is marked nonblocking and no connections are present to be accepted.
    POSIX.1-2001 allows either error to be returned for this case, and does not require
    these constants to have the same value, so a portable application should check for
    both possibilities.

    The resulting address can be None, which means that the connection
    didn't take place.

    The only way this can happen is if the accept() system call returned 0 in addrlen, which sounds rather strange. I'm not convinced hiding operating system bugs is a good idea.

    @giampaolo
    Copy link
    Contributor Author

    giampaolo commented Sep 25, 2010

    I'm not convinced hiding operating system bugs is a good idea.

    Do you propose to let the error raise then?
    The point of frameworks such as asyncore and twisted is to hide all system-specific errors as much as possible and provide a portable interface across all platforms.
    AFAICT, the whole point of this issue is that there should be only one way for an asyncore-based server to accept an incoming connection, possibly avoiding the user to deal with low-level details such as catching EWOULDBLOCK/ECONNABORTED/... in his application, and looking for accept() returning None is one possibility.

    As I said, in a better designed framework the user shouldn't be supposed to call accept() at all, but that's how asyncore is designed.

    @pitrou
    Copy link
    Member

    pitrou commented Sep 25, 2010

    The point of frameworks such as asyncore and twisted is to hide all
    system-specific errors as much as possible and provide a portable
    interface across all platforms.

    As long as these errors are reasonably explainable.
    If a strange error only occurs when running nmap on an OS X server, then
    it might be useful for the user to know that the OS X server isn't able
    to service all connections properly due to a bug in the operating
    system.

    AFAICT, the whole point of this issue is that there should be only one
    way for an asyncore-based server to accept an incoming connection,
    possibly avoiding the user to deal with low-level details such as
    catching EWOULDBLOCK/ECONNABORTED/...

    I am talking specifically about the address being None (i.e., a (sock,
    None) tuple is successfully returned).
    EWOULDBLOCK and ECONNABORTED are documented error conditions for
    accept(), but returning 0 in addrlen is not.

    As I said, in a better designed framework the user shouldn't be
    supposed to call accept() at all, but that's how asyncore is designed.

    Perhaps it's time to add an alternative handle_accepted(sock, addr)
    which relieves the user from calling accept() :))
    Then, perhaps self.accept() shouldn't filter any errors at all, but the
    default handle_accept() would silence them before calling
    handle_accepted().

    @giampaolo
    Copy link
    Contributor Author

    giampaolo commented Sep 25, 2010

    As long as these errors are reasonably explainable.
    If a strange error only occurs when running nmap on an OS X server,
    then it might be useful for the user to know that the OS X server
    isn't able to service all connections properly due to a bug in the
    operating system.

    It might be useful to find whether this is tracked somewhere.
    For the record, here (comment #10) is a code which should reproduce this problem:
    https://bugs.launchpad.net/zodb/+bug/135108
    As for what asyncore should in this case I think it should theoretically be safe for accept() to return (sock, None). What usually happen after that is that the socket object is passed to another dispatcher subclass (the handler) and the connection gets automatically closed once recv() or send() get called on the next async-loop.

    Perhaps it's time to add an alternative handle_accepted(sock, addr)
    which relieves the user from calling accept() :))

    This sounds like a very good idea for 3.2.
    As for 3.1 and 2.7 (2.6?) I think it can be useful to suppress EWOULDBLOCK/ECONNABORTED and return None instead.
    Considering accept() can *already* return None it wouldn't break backward compatibility.

    @giampaolo
    Copy link
    Contributor Author

    giampaolo commented Oct 2, 2010

    Patch in attachment adds a handled_accepted() method to dispatcher class as recommended by Antoine.

    @pitrou
    Copy link
    Member

    pitrou commented Oct 3, 2010

    I'm not an asyncore expert, but I can't see anything wrong with the patch.

    @giampaolo
    Copy link
    Contributor Author

    giampaolo commented Oct 4, 2010

    Python 3.2 changes committed in r85220.
    Still have to commit EWOULDBLOCK/ECONNABORTED changes for 3.1 and 2.7.

    @Arfrever
    Copy link
    Mannequin

    Arfrever mannequin commented Oct 30, 2010

    @giampaolo
    Copy link
    Contributor Author

    giampaolo commented Nov 1, 2010

    Fixed in r86084 (2.7) and r86085 (3.1).

    @giampaolo giampaolo added type-security A security issue and removed type-bug An unexpected behavior, bug, or error labels Nov 1, 2010
    @warsaw
    Copy link
    Member

    warsaw commented Dec 7, 2010

    I do not see this as a security bug so no patch for 2.6 please. (Comment requested from IRC).

    @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
    stdlib Python modules in the Lib dir type-security A security issue
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants