Opened 8 years ago

Closed 5 years ago

#1977 defect closed fixed (fixed)

unexpected search command breaks imap search

Reported by: tvachon Owned by:
Priority: high Milestone:
Component: mail Keywords: imap4
Cc: exarkun, tvachon Branch: branches/imap4-search-1977-2
(diff, github, buildbot, log)
Author: therve, itamar, exarkun Launchpad Bug:

Description

twisted.mail.imap4.IMAP4Server.singleSearchStep has code to handle unexpected search commands, but this code cannot be reached (line 1466):

            f = getattr(self, 'search_' + c)
            if f:
                if not f(query, id, msg):
                    return False
            else:
                # IMAP goes *out of its way* to be complex
                # Sequence sets to search should be specified
                # with a command, like EVERYTHING ELSE.
                try:
                    m = parseIdList(c)
                except:
                    log.err('Unknown search term: ' + c)
                else:
                    if id not in m:
                        return False

Since f = getattr(self, 'search_' + c) raises an AttributeError if 'search_' + c does not exist, f can never evaluate to False, so the "else" suite never runs.

The fix is to replace

f = getattr(self, 'search_' + c) 

with

f = getattr(self, 'search_' + c, None)

This problem arises from IMAP client commands like

00000003 SEARCH 1 UNDELETED UNSEEN

The only client I've seen use this is Pine, but since this does break compatibility with Pine, I've marked its priority as High.

Attachments (2)

ticket1977fix_against20060731svn.diff (412 bytes) - added by tvachon 8 years ago.
fix for this defect
ticket1977_unittest.diff (1.4 KB) - added by tvachon 8 years ago.
Unittest for this defect

Download all attachments as: .zip

Change History (21)

Changed 8 years ago by tvachon

fix for this defect

comment:1 Changed 8 years ago by tvachon

This and ticket #1978 are my first bug submissions to Twisted. I expect that just submission of these patches is not enough to get them included, and would be more than happy to follow up with whatever steps are needed to move the inclusion process along.

Thanks,

Travis Vachon

comment:2 Changed 8 years ago by glyph

These fixes will probably be included in due course. The thing you can do which will move them along the fastest is to write unit tests. No new code (even bugfixes!) goes into Twisted without tests: otherwise we don't have a rigorous way of determining that the bug was actually fixed, fixed on all platforms, remains fixed, etc.

Changed 8 years ago by tvachon

Unittest for this defect

comment:3 Changed 8 years ago by exarkun

  • Owner changed from exarkun to tvachon

Thanks for the patch and test. I modified the test a bit to actually assert something about the results from search(). Unfortunately, search support with message sets seems to be completely broken. The patch prevents an exception from being raised, but does not correctly restrict the result set.

Searching for 1 should return at most a result set containing 1. However, with Twisted's search implementation, it returns the entire contents of the mailbox. I'm not exactly sure why this is right now.

I've checked your patch and my modifications into a branch, source:/branches/imap4-search-1977, if you'd like to take a look and make further changes.

I probably won't get back to this for at least a week, quite possibly longer.

comment:4 Changed 8 years ago by exarkun

  • Cc exarkun tvachon added

btw, that link won't work until tomorrow.

comment:5 Changed 8 years ago by exarkun

  • Milestone set to Mail-0.4

comment:6 Changed 5 years ago by exarkun

  • Milestone Twisted-8.2+1 deleted

I can't see any reason this needs to be targeted at 9.0 (8.2+1).

comment:7 Changed 5 years ago by jpsimons

I ran into the same issue. The iPhone's IMAP client sends a search like 1:* NOT DELETED which ends up hitting this bug. The problem is you can fix it like you mention with a None default for get_attr, but then it's running new code than never ran before, which is itself buggy. My 1:* creates a MessageSet with an undefined last property, so contains ends up failing with the *. I patched it like this (there's a better way I'm sure):

            c = q.upper()
-           f = getattr(self, 'search_' + c)
+           f = getattr(self, 'search_' + c, None)
            if f:
                if not f(query, id, msg):
                    return False
            else:
                # IMAP goes *out of its way* to be complex
                # Sequence sets to search should be specified
                # with a command, like EVERYTHING ELSE.
                try:
                    m = parseIdList(c)
+                   m.last = 99999999999
                except:
                    log.err('Unknown search term: ' + c)
                else:
                    if id not in m:
                        return False

comment:8 Changed 5 years ago by exarkun

  • Keywords review added
  • Owner tvachon deleted

comment:9 Changed 5 years ago by exarkun

#4048 was a duplicate of this.

comment:10 Changed 5 years ago by therve

  • Author set to therve
  • Branch set to branches/imap4-search-1977-2

(In [27367]) Branching to 'imap4-search-1977-2'

comment:11 Changed 5 years ago by therve

I went on and had a look. The branch wasn't merging cleanly, and the test didn't pass anymore. I fixed it, and fix the other problem with '1:*'. I can't review that anymore, though.

comment:12 Changed 5 years ago by exarkun

  • Keywords review removed
  • Owner set to exarkun
  • Status changed from new to assigned

There are some IMAP4 issues. :/ Itamar and I will try to fix them.

comment:13 Changed 5 years ago by exarkun

  • Author changed from therve to therve, itamar, exarkun
  • Keywords review added
  • Owner exarkun deleted
  • Status changed from assigned to new

Okay! Hopefully things are a bit better now.

comment:14 Changed 5 years ago by therve

  • Keywords review removed
  • Owner set to exarkun

Alright! Following the great rule that I reviewed what I didn't write, please merge this one.

comment:15 Changed 5 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [27483]) Merge imap4-search-1977-2

Author: tvachon, therve, exarkun, itamar
Reviewer: exarkun, itamar, therve
Fixes: #1977

Fix twisted.mail.imap4.IMAP4Server's handling of the SEARCH command so that
it handles the bare <sequence set> case. Previously this would result in an
unhandled exception.

comment:16 Changed 5 years ago by exarkun

  • Resolution fixed deleted
  • Status changed from closed to reopened

(In [27489]) Revert r27483 - test suite regression on Python < 2.6

Reopens #1977

comment:17 Changed 5 years ago by exarkun

(In [27490]) Use zip range len + 1 instead of enumerate because Python 2.5 and earlier don't have two-argument enumerate

refs #1977

comment:18 Changed 5 years ago by exarkun

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [27499]) Merge imap4-search-1977-2

Author: tvachon, therve, exarkun, itamar
Reviewer: exarkun, itamar, therve
Fixes: #1977

Fix twisted.mail.imap4.IMAP4Server's handling of the SEARCH command so that
it handles the bare <sequence set> case. Previously this would result in an
unhandled exception.

This remerge replaces the use of a Python 2.6-only construct (two-arg enumerate)
with a more Python 2.3-friendly construct.

comment:19 Changed 3 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.