Opened 15 years ago

Closed 15 years ago

#1992 defect closed fixed (fixed)

IMAPServer.__cbFetch can raise uncaught AttributeError

Reported by: tvachon Owned by:
Priority: highest Milestone:
Component: mail Keywords:
Cc: Branch:
Author:

Description

twisted.mail.imap4.IMAPServer.__cbFetch will raise an AttributeError at line 1633 or 1634 in twisted/mail/imap4.py if:

  1. self.blocked is not None and
  2. self._oldTimeout has been deleted (with the del self._oldTimeout in a previous iteration of __cbFetch)

since the StopIteration exception will be raised whenever the iteration finishes (see below).

    def __cbFetch(self, results, tag, query, uid):
        if self.blocked is None:
            self.blocked = []
            self._oldTimeout = self.setTimeout(None)
        try:
            id, msg = results.next()
        except StopIteration:
            self.sendPositiveResponse(tag, 'FETCH completed')
            self._unblock()
            self.setTimeout(self._oldTimeout) # This statement can raise an Attribute Error
            del self._oldTimeout # And so can this one.
        else:
            self.spewMessage(id, msg, query, uid
                ).addCallback(lambda _: self.__cbFetch(results, tag, query, uid)
                ).addErrback(self.__ebSpewMessage
                )

This appears to be a rare case, but happens when large numbers of messages are added to my server via the Mac Mail client.

An easy fix to this is as follows:

    def __cbFetch(self, results, tag, query, uid):
        if self.blocked is None:
            self.blocked = []
            self._oldTimeout = self.setTimeout(None)
        try:
            id, msg = results.next()
        except StopIteration:
            self.sendPositiveResponse(tag, 'FETCH completed')
            self._unblock()
            self.setTimeout(getattr(self, "_oldTimeout", None)) # This takes care of this line
            try: # This try/except block should fix this line
                del self._oldTimeout
            except AttributeError:
                pass
        else:
            self.spewMessage(id, msg, query, uid
                ).addCallback(lambda _: self.__cbFetch(results, tag, query, uid)
                ).addErrback(self.__ebSpewMessage
                )

I'm attaching a patch for the fix, and will hopefully write unit tests within the next week (along with tests for tickets 1977 and 1978).

Attachments (1)

twisted-ticket-1992-20060808.diff (653 bytes) - added by tvachon 15 years ago.
Fix for this defect

Download all attachments as: .zip

Change History (10)

Changed 15 years ago by tvachon

Fix for this defect

comment:1 Changed 15 years ago by tvachon

Sorry, I was looking at an old version of imap4.py when I filed this. This defect appears on lines 1638 and 1639 in the latest subversion checkout.

comment:2 Changed 15 years ago by Jean-Paul Calderone

Owner: changed from Jean-Paul Calderone to tvachon

I suspect the attached patch will result in timeout support being lost at some point during the lifetime of a connection.

Here's an alternate proposal.

Index: twisted/mail/imap4.py
===================================================================
--- twisted/mail/imap4.py       (revision 17811)
+++ twisted/mail/imap4.py       (working copy)
@@ -1634,9 +1634,9 @@
             id, msg = results.next()
         except StopIteration:
             self.sendPositiveResponse(tag, 'FETCH completed')
-            self._unblock()
             self.setTimeout(self._oldTimeout)
             del self._oldTimeout
+            self._unblock()
         else:
             self.spewMessage(id, msg, query, uid
                 ).addCallback(lambda _: self.__cbFetch(results, tag, query, uid)

I believe what is happening is this:

The client sends two fetch commands in rapid succession. The first is parsed and dispatched to do_FETCH which results in __cbFetch which sets _oldTimeout and blocked before calling down to spewMessage which begins sending a response to the request. This may take several reactor iterations to complete, due to the use of iterateInReactor to handle the potentially larger sections of the response. Meanwhile the second fetch gets blocked and queued. Eventually the spewMessage Deferred fires and __cbFetch is invoked again. This time blocked is not None so the first suite is skipped. results.next() raises a StopIteration and the except suite runs. _unblock() runs which parses the 2nd queued fetch request, going down into do_FETCH and then __cbFetch. This time self.blocked is None so it is set to an empty list and _oldTimeout is set (to None, incorrectly). unblock returns and self.setTimeout(self._oldTimeout) runs, followed by del self._oldTimeout. At this point the timeout setting is correct but the remembered timeout value has been destroyed. When the 2nd fetch request finishes and follows the StopIteration except suite, it triggers the AttributeError.

The patch above should resolve this by sorting out the re-entrancy issues and avoiding corrupting the instance state.

Of course, a unit test for this case would be most helpful.

comment:3 Changed 15 years ago by tvachon

Thanks for the comments, I figured my fix might be a little shortsighted. Your fix does appear to have fixed my bug, thanks!

I've been working on a unittest for this defect for about 6 hours now, and haven't made a whole lot of progress. The bug appears in the wild after a pretty long string of

[tag] UID FETCH [number] BODY.PEEK[]

commands, and happens fairly randomly. Do you have any suggestions on how this might be tested?

Thanks very much!

comment:4 Changed 15 years ago by Jean-Paul Calderone

I'll have a go at writing the test for this, I think I know how to do it.

Thanks for confirming that this fixes the problem for you.

comment:5 Changed 15 years ago by Jean-Paul Calderone

Owner: changed from tvachon to Jean-Paul Calderone
Status: newassigned

comment:6 Changed 15 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted
Priority: highhighest
Status: assignednew

Ready for review in fetch-pipeline-1992

comment:7 Changed 15 years ago by radix

Keywords: review removed
Owner: set to Jean-Paul Calderone

The test is pretty creepy, but is sufficiently documented.

The code in that StopIteration except suite is pretty dense; a comment documenting the ordering requirements would be good.

Please merge.

comment:8 Changed 15 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [17925]) Merge fetch-pipeline-1992

Author: exarkun Reviewer: radix Fixes #1992

This fixes a state corruption bug in the IMAP4 server which manifest when a client pipelined three or more FETCH commands which included a request for message text. The server is now careful to avoid re-entrancy issues when handling pipelining. A test for this case has also been added, and the IMAP4 server's scheduler is now parameterized.

Thanks to tvachon for pointing out this bug.

comment:9 Changed 11 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.