[issue6706] asyncore's accept() is broken
Barry A. Warsaw ba...@python.org added the comment: I do not see this as a security bug so no patch for 2.6 please. (Comment requested from IRC). -- nosy: +barry ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Giampaolo Rodola' g.rod...@gmail.com added the comment: Fixed in r86084 (2.7) and r86085 (3.1). -- resolution: - fixed stage: patch review - committed/rejected status: open - closed type: behavior - security versions: +Python 2.7, Python 3.1 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Changes by Arfrever Frehtes Taifersar Arahesis arfrever@gmail.com: -- nosy: +Arfrever ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Arfrever Frehtes Taifersar Arahesis arfrever@gmail.com added the comment: CVE-2010-3492 references this issue. http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2010-3492 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Giampaolo Rodola' g.rod...@gmail.com added the comment: Python 3.2 changes committed in r85220. Still have to commit EWOULDBLOCK/ECONNABORTED changes for 3.1 and 2.7. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Antoine Pitrou pit...@free.fr added the comment: I'm not an asyncore expert, but I can't see anything wrong with the patch. -- stage: needs patch - patch review versions: -Python 2.6, Python 2.7, Python 3.1 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Giampaolo Rodola' g.rod...@gmail.com added the comment: Patch in attachment adds a handled_accepted() method to dispatcher class as recommended by Antoine. -- Added file: http://bugs.python.org/file19104/accept.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Giampaolo Rodola' g.rod...@gmail.com added the comment: 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 . -- versions: +Python 2.6 Added file: http://bugs.python.org/file19005/accept.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Antoine Pitrou pit...@free.fr added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Giampaolo Rodola' g.rod...@gmail.com added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Antoine Pitrou pit...@free.fr added the comment: 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(). -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Giampaolo Rodola' g.rod...@gmail.com added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Giampaolo Rodola' g.rod...@gmail.com added the comment: 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. -- keywords: +patch nosy: +pitrou Added file: http://bugs.python.org/file19001/accept.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Dave Malcolm dmalc...@redhat.com added the comment: 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 -- nosy: +dmalcolm ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Changes by jan matejek jmate...@suse.cz: -- nosy: +matejcik ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Changes by Santoso Wijaya santa@me.com: -- nosy: +santa4nt ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Giampaolo Rodola' g.rod...@gmail.com added the comment: 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. -- assignee: - giampaolo.rodola ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
Mark Lawrence breamore...@yahoo.co.uk added the comment: @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. -- nosy: +BreamoreBoy stage: - needs patch type: - behavior versions: +Python 3.1, Python 3.2 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue6706] asyncore's accept() is broken
New submission from Giampaolo Rodola' billiej...@users.sourceforge.net: 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: - self.accept() can return None instead of a socket pair in which case TypeError is raised (see pyftpdlib bug: http://code.google.com/p/pyftpdlib/issues/detail?id=91) - ECONNABORTED can be raised. This is reproducible on Linux by hammering the server with nmap (see pyftpdlib bug: http://code.google.com/p/pyftpdlib/issues/detail?id=105) - EAGAIN can be raised too. I never experienced this error condition myself in pyftpdlib but by looking at twisted/internet/tcp.py it seems reasonable to catch EAGAIN too. - The resulting address can be None, which means that the connection didn't take place. This is reproducible by hammering the server with nmap (see pyftpdlib bug http://code.google.com/p/pyftpdlib/issues/detail?id=104). 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() -- components: Library (Lib) messages: 91579 nosy: giampaolo.rodola, josiahcarlson severity: normal status: open title: asyncore's accept() is broken versions: Python 2.7 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6706 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
