Re: [Zope3-dev] DoS problem in zope.sendmail.maildir
On Thu, Jun 07, 2007 at 12:52:43PM +0200, Sascha Ottolski wrote: > Am Mittwoch 06 Juni 2007 schrieb Marius Gedminas: > > I'm attaching the patch here. I'd be happy to commit it to trunk if > > somebody else could test it on various platforms. (I haven't even > > tested if it works fine on Linux, other than by running the unit > > tests.) > > Marius, > > again, thanks a lot for the patch. I can confirm that it works fine > here. I also checked that e.errno == errno.EEXIST is True on Windows > XP. Thanks. Fix committed in rev 76463. Marius Gedminas -- Of course everyone knows that vim is the best text editor in the world. Anyone who tells you differently is either wrong, lying, or criminally insane. (Or an emacs user, in which case they are wrong, lying and criminally insane). signature.asc Description: Digital signature ___ Zope3-dev mailing list Zope3-dev@zope.org Unsub: http://mail.zope.org/mailman/options/zope3-dev/archive%40mail-archive.com
Re: [Zope3-dev] DoS problem in zope.sendmail.maildir
Am Mittwoch 06 Juni 2007 schrieb Marius Gedminas: > I'm attaching the patch here. I'd be happy to commit it to trunk if > somebody else could test it on various platforms. (I haven't even > tested if it works fine on Linux, other than by running the unit > tests.) Marius, again, thanks a lot for the patch. I can confirm that it works fine here. I also checked that e.errno == errno.EEXIST is True on Windows XP. Cheers, Sascha -- Lalisio - connecting knowledge worldwide Lalisio GmbH Puschkinstraße 1 99084 Erfurt fon +49 (0)361 541 43 80 fax +49 (0)361 541 43 79 [EMAIL PROTECTED] www.lalisio.com Sitz der Gesellschaft: Erfurt Amtsgericht Jena, HRB 113943 Geschäftsführerin: Ute Rother Ust-Id.: DE813237441 ___ Zope3-dev mailing list Zope3-dev@zope.org Unsub: http://mail.zope.org/mailman/options/zope3-dev/archive%40mail-archive.com
Re: [Zope3-dev] DoS problem in zope.sendmail.maildir
Am Mittwoch 06 Juni 2007 schrieb Marius Gedminas: > > BTW, I'm wondering if it is really safe to assume that > > os.rename(self._filename, self._new_filename) in the commit() > > message never fails? > > Nothing is really safe. Renaming a file is much safer than creating > a file. > > > Although I believe that it's almost impossible that it > > really could :-) > > Sudden filesystem corruption, the kernel remounts it as read-only, > and you cannot rename anything. But then you have bigger problems > than an exception in the second phase of a commit. I see :-) While I was more thinking of the very unlikey situation of a "file exists" situation. Thanks very much for you lightning fast effort, I will test it right away and report back (I also just booted a windows machine to check if errno.EEXIST works there as well). I may also file a launchpad bug including your patch, if you want me to. Cheers, Sascha -- Lalisio - connecting knowledge worldwide Lalisio GmbH Puschkinstraße 1 99084 Erfurt fon +49 (0)361 541 43 80 fax +49 (0)361 541 43 79 [EMAIL PROTECTED] www.lalisio.com Sitz der Gesellschaft: Erfurt Amtsgericht Jena, HRB 113943 Geschäftsführerin: Ute Rother Ust-Id.: DE813237441 AKTUELLER HINWEIS (Juni 2007) "Wen hätten Sie gerne im Wissensnetzwerk von Lalisio dabei?" Wir laden Sie herzlich ein, uns im Rahmen unseres Online-Events bis zum 31. Juli 2007 Ihre Wunschkandidaten für unser Wissensnetzwerk zu nennen. Ganz gleich, ob ehemalige Kommilitonen, geschätzte Kollegen oder Vorbilder aus Wissenschaft und Forschung - wir begeben uns auf die Suche und finden Wunschkandidaten zum Wissensaustausch. Um ein Expertengesuch abzugeben, besuchen Sie uns unter: www.lalisio.com ___ Zope3-dev mailing list Zope3-dev@zope.org Unsub: http://mail.zope.org/mailman/options/zope3-dev/archive%40mail-archive.com
Re: [Zope3-dev] DoS problem in zope.sendmail.maildir
On Thu, Jun 07, 2007 at 12:36:25AM +0300, Marius Gedminas wrote: > On Wed, Jun 06, 2007 at 06:04:50PM +0200, Sascha Ottolski wrote: > > I discovered a problem with the maildir implementation, that is > > triggered when trying to use the QueuedDelivery with many (read: more > > than current filedescriptor limit of the zope process) mails at once: > ... > > OSError: [Errno 24] Too many open files: > > '/home/so/sandbox/var/zope/lib/python/lalisio/app/site/../../../../var/zope/var/mqueue/new' ... > > The reason for that to happen is clear to me, however, I'm not sure how > > a fix could look like. > > Did you create an bug in launchpad? I could attach a patch that I think > would fix the problem, but I don't currently have the time and energy to > test myself. I'm attaching the patch here. I'd be happy to commit it to trunk if somebody else could test it on various platforms. (I haven't even tested if it works fine on Linux, other than by running the unit tests.) Marius Gedminas -- (mental note: stop installing red hat. everytime i do so, it takes ages to fix my system again.) -- from the sig of Martin H�gman Make QueuedMailDelivery not keep open files around for every message sent during a transaction. Patch by Marius Gedminas <[EMAIL PROTECTED]> Index: tests/test_maildir.py === --- tests/test_maildir.py (revision 75881) +++ tests/test_maildir.py (working copy) @@ -19,6 +19,7 @@ import unittest import stat import os +import errno from zope.interface.verify import verifyObject @@ -123,7 +124,7 @@ def open(self, filename, flags, mode=0777): if (flags & os.O_EXCL and flags & os.O_CREAT and self.access(filename, 0)): -raise OSError('file already exists') +raise OSError(errno.EEXIST, 'file already exists') if not flags & os.O_CREAT and not self.access(filename, 0): raise OSError('file not found') fd = max(self._descriptors.keys() + [2]) + 1 Index: tests/test_delivery.py === --- tests/test_delivery.py (revision 75881) +++ tests/test_delivery.py (working copy) @@ -147,18 +147,32 @@ data = '' commited_messages = [] # this list is shared among all instances aborted_messages = [] # this one too +_closed = False def write(self, str): +if self._closed: +raise AssertionError('already closed') self.data += str def writelines(self, seq): +if self._closed: +raise AssertionError('already closed') self.data += ''.join(seq) +def close(self): +self._closed = True + def commit(self): +if not self._closed: +raise AssertionError('for this test we want the message explicitly' + ' closed before it is committed') self._commited = True self.commited_messages.append(self.data) def abort(self): +if not self._closed: +raise AssertionError('for this test we want the message explicitly' + ' closed before it is committed') self._aborted = True self.aborted_messages.append(self.data) Index: maildir.py === --- maildir.py (revision 75881) +++ maildir.py (working copy) @@ -18,6 +18,7 @@ __docformat__ = 'restructuredtext' import os +import errno import socket import time import random @@ -91,7 +92,9 @@ filename = join(subdir_tmp, unique) try: fd = os.open(filename, os.O_CREAT|os.O_EXCL|os.O_WRONLY, 0600) -except OSError: +except OSError, e: +if e.errno != errno.EEXIST: +raise # File exists counter += 1 if counter >= 1000: @@ -115,7 +118,7 @@ self._filename = filename self._new_filename = new_filename self._fd = fd -self._closed = False +self._finished = False self._aborted = False def write(self, data): @@ -124,22 +127,28 @@ def writelines(self, lines): self._fd.writelines(lines) +def close(self): +self._fd.close() + def commit(self): -if self._closed and self._aborted: +if self._aborted: raise RuntimeError('Cannot commit, message already aborted') -elif not self._closed: -self._closed = True -self._aborted = False +elif not self._finished: +self._finished = True self._fd.close() os.rename(self._filename, self._new_filename) # NOTE: the same maildir.html says it should be a link, followed by # unlink. But Win32 does not necessarily have hardlinks! def abort(self): -if not self._close
Re: [Zope3-dev] DoS problem in zope.sendmail.maildir
Hi, On Wed, Jun 06, 2007 at 06:04:50PM +0200, Sascha Ottolski wrote: > I discovered a problem with the maildir implementation, that is > triggered when trying to use the QueuedDelivery with many (read: more > than current filedescriptor limit of the zope process) mails at once: ... > OSError: [Errno 24] Too many open files: > '/home/so/sandbox/var/zope/lib/python/lalisio/app/site/../../../../var/zope/var/mqueue/new' Oh, wow. I'd forgotted that this causes the data manager to keep an open file descriptor for every email. Ouch. Suggestion: introduce IMaildirMessageWriter.close() and call it in QueuedMailDelivery.createDataManager. > I need to kill -9 the server after that, SIGTERM seems to be ignored. Ouch. > The reason for that to happen is clear to me, however, I'm not sure how > a fix could look like. Did you create an bug in launchpad? I could attach a patch that I think would fix the problem, but I don't currently have the time and energy to test myself. > One source of the problem is > > > try: > fd = os.open(filename, os.O_CREAT|os.O_EXCL|os.O_WRONLY, 0600) > except OSError: > # File exists > > > OSError is obviously to broad to catch as a test if the unique filename > could be created. May be use os.path.exists(filename) instead? But I'm > not sure if this is equivalent in a platform independant way. That solution would have a race condition. Better try: fd = os.open(filename, os.O_CREAT|os.O_EXCL|os.O_WRONLY, 0600) except OSError, e: import errno # actually put this at the very top if e.errno != errno.EEXIST: raise # File exists but would it work on Windows? Could anyone test that by running this little program on Windows: import os, errno file('tempfile', 'w').close() try: fd = os.open('tempfile', os.O_CREAT|os.O_EXCL|os.O_WRONLY, 0600) except OSError, e: print e.errno == errno.EEXIST # prints True on Linux > The real source of the problem lies in the MaildirMessageWriter: The > passed-in filedescriptor remains open until commit() is called. No > problem for sending some mails only, but when sending lots of > messages in one transaction, the filedescriptor limit kicks in. It's scary to see the problems in zope.sendmail, given that I'm at least partially responsible for all of them. > A dirty solution might be to change the write() and writeline() to open > and close the file on every call. But there's probably a saner way. Yes. > BTW, I'm wondering if it is really safe to assume that > os.rename(self._filename, self._new_filename) in the commit() message > never fails? Nothing is really safe. Renaming a file is much safer than creating a file. > Although I believe that it's almost impossible that it > really could :-) Sudden filesystem corruption, the kernel remounts it as read-only, and you cannot rename anything. But then you have bigger problems than an exception in the second phase of a commit. Marius Gedminas -- For Sale: Parachute. Only used once, never opened, small stain. signature.asc Description: Digital signature ___ Zope3-dev mailing list Zope3-dev@zope.org Unsub: http://mail.zope.org/mailman/options/zope3-dev/archive%40mail-archive.com
[Zope3-dev] DoS problem in zope.sendmail.maildir
Hi, I discovered a problem with the maildir implementation, that is triggered when trying to use the QueuedDelivery with many (read: more than current filedescriptor limit of the zope process) mails at once: Exception in thread Thread-1: Traceback (most recent call last): File "/usr/lib/python2.4/threading.py", line 442, in __bootstrap self.run() File "/home/so/sandbox/app/zope3/lib/python/zope/sendmail/delivery.py", line 193, in run File "/home/so/sandbox/app/Zope-3.3.0/lib/python/zope/sendmail/maildir.py", line 69, in __iter__ OSError: [Errno 24] Too many open files: '/home/so/sandbox/var/zope/lib/python/lalisio/app/site/../../../../var/zope/var/mqueue/new' Which then seems to make the whole server blocking; trying to open a page on the the server results in: Traceback (most recent call last): File "/home/so/sandbox/app/zope3/lib/python/twisted/python/log.py", line 43, in callWithContext File "/home/so/sandbox/app/zope3/lib/python/twisted/python/context.py", line 59, in callWithContext return self.currentContext().callWithContext(ctx, func, *args, **kw) File "/home/so/sandbox/app/zope3/lib/python/twisted/python/context.py", line 37, in callWithContext return func(*args,**kw) File "/home/so/sandbox/app/zope3/lib/python/twisted/internet/selectreactor.py", line 139, in _doReadOrWrite --- --- File "/home/so/sandbox/app/zope3/lib/python/twisted/internet/tcp.py", line 753, in doRead File "/usr/lib/python2.4/socket.py", line 161, in accept socket.error: (24, 'Too many open files') I need to kill -9 the server after that, SIGTERM seems to be ignored. The reason for that to happen is clear to me, however, I'm not sure how a fix could look like. One source of the problem is try: fd = os.open(filename, os.O_CREAT|os.O_EXCL|os.O_WRONLY, 0600) except OSError: # File exists OSError is obviously to broad to catch as a test if the unique filename could be created. May be use os.path.exists(filename) instead? But I'm not sure if this is equivalent in a platform independant way. The real source of the problem lies in the MaildirMessageWriter: The passed-in filedescriptor remains open until commit() is called. No problem for sending some mails only, but when sending lots of messages in one transaction, the filedescriptor limit kicks in. A dirty solution might be to change the write() and writeline() to open and close the file on every call. But there's probably a saner way. BTW, I'm wondering if it is really safe to assume that os.rename(self._filename, self._new_filename) in the commit() message never fails? Although I believe that it's almost impossible that it really could :-) Thanks, Sascha -- Lalisio - connecting knowledge worldwide Lalisio GmbH Puschkinstraße 1 99084 Erfurt fon +49 (0)361 541 43 80 fax +49 (0)361 541 43 79 [EMAIL PROTECTED] www.lalisio.com Sitz der Gesellschaft: Erfurt Amtsgericht Jena, HRB 113943 Geschäftsführerin: Ute Rother Ust-Id.: DE813237441 ___ Zope3-dev mailing list Zope3-dev@zope.org Unsub: http://mail.zope.org/mailman/options/zope3-dev/archive%40mail-archive.com