From 69304f822824d2def7719b4272191de18d556996 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Sat, 2 May 2020 15:11:36 +1000 Subject: [PATCH 1/4] Geary.Endpoint: Move max batch size quirk config to quirks object --- src/engine/api/geary-endpoint.vala | 8 -------- src/engine/api/geary-engine.vala | 15 --------------- src/engine/imap/api/imap-client-service.vala | 12 ++++++++++++ src/engine/imap/api/imap-quirks.vala | 8 ++++++++ .../imap/transport/imap-client-session.vala | 2 +- 5 files changed, 21 insertions(+), 24 deletions(-) diff --git a/src/engine/api/geary-endpoint.vala b/src/engine/api/geary-endpoint.vala index c833c92d..f986ed61 100644 --- a/src/engine/api/geary-endpoint.vala +++ b/src/engine/api/geary-endpoint.vala @@ -71,14 +71,6 @@ public class Geary.Endpoint : BaseObject { get; set; default = TlsCertificateFlags.VALIDATE_ALL; } - /** - * The maximum number of commands that will be pipelined at once. - * - * If 0 (the default), there is no limit on the number of - * pipelined commands sent to this endpoint. - */ - public uint max_pipeline_batch_size = 0; - /** * When set, TLS has reported certificate issues. * diff --git a/src/engine/api/geary-engine.vala b/src/engine/api/geary-engine.vala index 5f8f7143..c91b67ff 100644 --- a/src/engine/api/geary-engine.vala +++ b/src/engine/api/geary-engine.vala @@ -426,22 +426,7 @@ public class Geary.Engine : BaseObject { uint timeout = service.protocol == Protocol.IMAP ? Imap.ClientConnection.RECOMMENDED_TIMEOUT_SEC : Smtp.ClientConnection.DEFAULT_TIMEOUT_SEC; - shared = new_endpoint(provider, service, timeout); - - // XXX this is pretty hacky, move this back into the - // OutlookAccount somehow - if (provider == ServiceProvider.OUTLOOK) { - // As of June 2016, outlook.com's IMAP servers have a bug - // where a large number (~50) of pipelined STATUS commands on - // mailboxes with many messages will eventually cause it to - // break command parsing and return a BAD response, causing us - // to drop the connection. Limit the number of pipelined - // commands per batch to work around this. See b.g.o Bug - // 766552 - shared.max_pipeline_batch_size = 25; - } - this.shared_endpoints.set(key, new EndpointWeakRef(shared)); } diff --git a/src/engine/imap/api/imap-client-service.vala b/src/engine/imap/api/imap-client-service.vala index 419c0b1d..21a2e4aa 100644 --- a/src/engine/imap/api/imap-client-service.vala +++ b/src/engine/imap/api/imap-client-service.vala @@ -49,6 +49,18 @@ public class Geary.Imap.ClientService : Geary.ClientService { // around the former. See #746 quirks.flag_atom_exceptions = "]"; break; + + case ServiceProvider.OUTLOOK: + // As of June 2016, outlook.com's IMAP servers have a bug + // where a large number (~50) of pipelined STATUS commands + // on mailboxes with many messages will eventually cause + // it to break command parsing and return a BAD response, + // causing us to drop the connection. Limit the number of + // pipelined commands per batch to work around this. See + // b.g.o Bug 766552 + quirks.max_pipeline_batch_size = 25; + break; + default: // noop break; diff --git a/src/engine/imap/api/imap-quirks.vala b/src/engine/imap/api/imap-quirks.vala index cce53d46..2c1a70c6 100644 --- a/src/engine/imap/api/imap-quirks.vala +++ b/src/engine/imap/api/imap-quirks.vala @@ -14,5 +14,13 @@ public class Geary.Imap.Quirks : BaseObject { /** The set of additional characters allowed in an IMAP flag. */ public string? flag_atom_exceptions { get; set; } + /** + * The maximum number of commands that will be pipelined at once. + * + * If 0 (the default), there is no limit on the number of + * pipelined commands sent to this endpoint. + */ + public uint max_pipeline_batch_size { get; set; default = 0; } + } diff --git a/src/engine/imap/transport/imap-client-session.vala b/src/engine/imap/transport/imap-client-session.vala index 13957422..f3f30a10 100644 --- a/src/engine/imap/transport/imap-client-session.vala +++ b/src/engine/imap/transport/imap-client-session.vala @@ -1290,7 +1290,7 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source { // the endpoint's max pipeline size is positive, if so use // multiple batches with a maximum size of that. - uint max_batch_size = this.imap_endpoint.max_pipeline_batch_size; + uint max_batch_size = this.quirks.max_pipeline_batch_size; if (max_batch_size < 1) { max_batch_size = cmds.size; } From 16b7d6528d6ce744ce894da9aacd009a90780276 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Sat, 2 May 2020 15:39:54 +1000 Subject: [PATCH 2/4] Geary.Imap.FolderSession: Move fetch header quirk to Quirks object --- src/engine/imap/api/imap-folder-session.vala | 16 ++++++---------- src/engine/imap/api/imap-quirks.vala | 18 +++++++++++++++++- .../imap/transport/imap-client-session.vala | 4 +++- 3 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/engine/imap/api/imap-folder-session.vala b/src/engine/imap/api/imap-folder-session.vala index c94da4bc..d1450755 100644 --- a/src/engine/imap/api/imap-folder-session.vala +++ b/src/engine/imap/api/imap-folder-session.vala @@ -38,13 +38,7 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject { /** Determines if this folder accepts custom IMAP flags. */ public Trillian accepts_user_flags { get; private set; default = Trillian.UNKNOWN; } - /** - * Set to true when it's detected that the server doesn't allow a - * space between "header.fields" and the list of email headers to - * be requested via FETCH; see: - * [[https://bugzilla.gnome.org/show_bug.cgi?id=714902|Bug * 714902]] - */ - public bool imap_header_fields_hack { get; private set; default = false; } + private Quirks quirks; private Nonblocking.Mutex cmd_mutex = new Nonblocking.Mutex(); private Gee.HashMap? fetch_accumulator = null; @@ -93,6 +87,7 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject { throws Error { base(session); this.folder = folder; + this.quirks = session.quirks; if (folder.properties.attrs.is_no_select) { throw new ImapError.NOT_SUPPORTED( @@ -388,7 +383,8 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject { // // See if (!header_fields.is_empty) { - if (!this.imap_header_fields_hack || header_fields.size == 1) { + if (!this.quirks.fetch_header_part_no_space || + header_fields.size == 1) { header_specifiers = new FetchBodyDataSpecifier[1]; header_specifiers[0] = new FetchBodyDataSpecifier.peek( FetchBodyDataSpecifier.SectionPart.HEADER_FIELDS, @@ -412,7 +408,7 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject { } foreach (FetchBodyDataSpecifier header in header_specifiers) { - if (this.imap_header_fields_hack) { + if (this.quirks.fetch_header_part_no_space) { header.omit_request_header_fields_space(); } cmds.add(new FetchCommand.body_data_type(msg_set, header)); @@ -1119,7 +1115,7 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject { // property was enabled after sending command but // before response returned if (specifier.request_header_fields_space) { - this.imap_header_fields_hack = true; + this.quirks.fetch_header_part_no_space = true; return true; } } diff --git a/src/engine/imap/api/imap-quirks.vala b/src/engine/imap/api/imap-quirks.vala index 2c1a70c6..79ef3267 100644 --- a/src/engine/imap/api/imap-quirks.vala +++ b/src/engine/imap/api/imap-quirks.vala @@ -1,5 +1,5 @@ /* - * Copyright © 2019-2020 Michael Gratton + * Copyright © 2020 Michael Gratton * * This software is licensed under the GNU Lesser General Public License * (version 2.1 or later). See the COPYING file in this distribution. @@ -11,6 +11,22 @@ public class Geary.Imap.Quirks : BaseObject { + /** + * Whether spaces are disallowed in header parts of fetch commands. + * + * If true, HEADER parts of a BODY section may not contain any + * spaces. + * + * E.g. this conformant form is not supported: + * + * a008 UID FETCH * BODY.PEEK[HEADER.FIELDS (REFERENCES)] + * + * Whereas this non-conformant form is supported: + * + * a008 UID FETCH * BODY.PEEK[HEADER.FIELDS(REFERENCES)] + */ + public bool fetch_header_part_no_space { get; set; default = false; } + /** The set of additional characters allowed in an IMAP flag. */ public string? flag_atom_exceptions { get; set; } diff --git a/src/engine/imap/transport/imap-client-session.vala b/src/engine/imap/transport/imap-client-session.vala index f3f30a10..68e6f06e 100644 --- a/src/engine/imap/transport/imap-client-session.vala +++ b/src/engine/imap/transport/imap-client-session.vala @@ -286,6 +286,9 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source { /** Records the actual name and delimiter used for the inbox */ internal MailboxInformation? inbox { get; private set; default = null; } + /** The quirks being used by this session. */ + internal Quirks quirks { get; set; } + // Locations personal mailboxes for this session private Gee.List personal_namespaces = new Gee.ArrayList(); @@ -296,7 +299,6 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source { private Gee.List shared_namespaces = new Gee.ArrayList(); private Endpoint imap_endpoint; - private Quirks quirks; private Geary.State.Machine fsm; private ClientConnection? cx = null; From d6d36768f9cfbc1f9df01d9596403bc181ba4740 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Sat, 2 May 2020 17:13:54 +1000 Subject: [PATCH 3/4] Geary.Imap.Deserialiser: Handle reserved chars in response-text RFC 3501 allows any kind of char except CRLF in `resp-text` after an optional response code, so handle that. Addresses another issue in #711 --- .../imap/transport/imap-deserializer.vala | 30 ++++++++++++++++--- .../transport/imap-deserializer-test.vala | 18 +++++++++-- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/src/engine/imap/transport/imap-deserializer.vala b/src/engine/imap/transport/imap-deserializer.vala index 72e739eb..70079e09 100644 --- a/src/engine/imap/transport/imap-deserializer.vala +++ b/src/engine/imap/transport/imap-deserializer.vala @@ -30,6 +30,10 @@ public class Geary.Imap.Deserializer : BaseObject, Logging.Source { private const size_t MAX_BLOCK_READ_SIZE = 4096; + private const string[] RESPONSE_ATOMS = { + "OK", "NO", "BAD", "BYE", "PREAUTH" + }; + private enum Mode { LINE, BLOCK, @@ -49,6 +53,7 @@ public class Geary.Imap.Deserializer : BaseObject, Logging.Source { LITERAL, LITERAL_DATA_BEGIN, LITERAL_DATA, + RESPONSE_TEXT, FAILED, CLOSED, COUNT @@ -102,6 +107,7 @@ public class Geary.Imap.Deserializer : BaseObject, Logging.Source { private Geary.Memory.GrowableBuffer? block_buffer = null; private unowned uint8[]? current_buffer = null; private int ins_priority = Priority.DEFAULT; + private bool is_parsing_flags = false; @@ -179,12 +185,12 @@ public class Geary.Imap.Deserializer : BaseObject, Logging.Source { new Geary.State.Mapping(State.START_PARAM, Event.ERROR, on_error), new Geary.State.Mapping(State.ATOM, Event.CHAR, on_atom_char), - new Geary.State.Mapping(State.ATOM, Event.EOL, on_atom_eol), + new Geary.State.Mapping(State.ATOM, Event.EOL, on_param_eol), new Geary.State.Mapping(State.ATOM, Event.EOS, on_eos), new Geary.State.Mapping(State.ATOM, Event.ERROR, on_error), new Geary.State.Mapping(State.FLAG, Event.CHAR, on_flag_char), - new Geary.State.Mapping(State.FLAG, Event.EOL, on_atom_eol), + new Geary.State.Mapping(State.FLAG, Event.EOL, on_param_eol), new Geary.State.Mapping(State.FLAG, Event.EOS, on_eos), new Geary.State.Mapping(State.FLAG, Event.ERROR, on_error), @@ -217,6 +223,11 @@ public class Geary.Imap.Deserializer : BaseObject, Logging.Source { new Geary.State.Mapping(State.LITERAL_DATA, Event.EOS, on_eos), new Geary.State.Mapping(State.LITERAL_DATA, Event.ERROR, on_error), + new Geary.State.Mapping(State.RESPONSE_TEXT, Event.CHAR, on_response_text_char), + new Geary.State.Mapping(State.RESPONSE_TEXT, Event.EOL, on_param_eol), + new Geary.State.Mapping(State.RESPONSE_TEXT, Event.EOS, on_eos), + new Geary.State.Mapping(State.RESPONSE_TEXT, Event.ERROR, on_error), + new Geary.State.Mapping(State.FAILED, Event.EOL, on_failed_eol), new Geary.State.Mapping(State.FAILED, Event.EOS, Geary.State.nop), new Geary.State.Mapping(State.FAILED, Event.ERROR, Geary.State.nop), @@ -590,7 +601,12 @@ public class Geary.Imap.Deserializer : BaseObject, Logging.Source { return State.START_PARAM; default: - if (!this.is_parsing_flags) { + if (this.context_stack.size == 1 && + this.context.size >= 2 && + this.context.get(1).to_string().ascii_up() in RESPONSE_ATOMS) { + append_to_string(ch); + return State.RESPONSE_TEXT; + } else if (!this.is_parsing_flags) { if (DataFormat.is_atom_special(ch)) { warning("Received an invalid atom-char: %c", ch); return State.FAILED; @@ -697,7 +713,7 @@ public class Geary.Imap.Deserializer : BaseObject, Logging.Source { return State.TAG; } - private uint on_atom_eol(uint state, uint event, void *user) { + private uint on_param_eol(uint state, uint event, void *user) { // clean up final atom save_string_parameter(false); flush_params(); @@ -837,6 +853,12 @@ public class Geary.Imap.Deserializer : BaseObject, Logging.Source { return State.START_PARAM; } + private uint on_response_text_char(uint state, uint event, void *user) { + char ch = *((char *) user); + append_to_string(ch); + return State.RESPONSE_TEXT; + } + private uint on_eos() { debug("EOS"); diff --git a/test/engine/imap/transport/imap-deserializer-test.vala b/test/engine/imap/transport/imap-deserializer-test.vala index 2f3a2d4a..b3a61712 100644 --- a/test/engine/imap/transport/imap-deserializer-test.vala +++ b/test/engine/imap/transport/imap-deserializer-test.vala @@ -47,6 +47,9 @@ class Geary.Imap.DeserializerTest : TestCase { // fail, disable for the moment add_test("invalid_flag_prefix", invalid_flag_prefix); + add_test("reserved_in_response_text", reserved_in_response_text); + + add_test("instant_eos", instant_eos); add_test("bye_eos", bye_eos); } @@ -220,14 +223,13 @@ class Geary.Imap.DeserializerTest : TestCase { public void aliyun_greeting() throws Error { string greeting = "* OK AliYun IMAP Server Ready(10.147.40.164)"; - string parsed = "* OK AliYun IMAP Server Ready (10.147.40.164)"; this.stream.add_data(greeting.data); this.stream.add_data(EOL.data); this.process.begin(Expect.MESSAGE, this.async_completion); RootParameters? message = this.process.end(async_result()); - assert(message.to_string() == parsed); + assert(message.to_string() == greeting); } public void invalid_atom_prefix() throws Error { @@ -320,6 +322,18 @@ class Geary.Imap.DeserializerTest : TestCase { //this.process.end(async_result()); } + public void reserved_in_response_text() throws Error { + // As seen in #711 + string line = """a008 BAD Missing ] in: header.fields"""; + this.stream.add_data(line.data); + this.stream.add_data(EOL.data); + + this.process.begin(Expect.MESSAGE, this.async_completion); + RootParameters? message = this.process.end(async_result()); + + assert(message.to_string() == line); + } + public void instant_eos() throws Error { this.process.begin(Expect.EOS, this.async_completion); this.process.end(async_result()); From 471d7237821c305a4352bf1edbcb7e2a5d6c1d26 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Wed, 18 Mar 2020 23:59:35 +1100 Subject: [PATCH 4/4] imap: Ignore UIDNEXT 0 values sent from servers Some mail servers e.g hMailServer and whatever is used by home.pl (dovecot?) sends UIDNEXT 0. Just ignore these since there nothing else that can be done. See #711 --- src/engine/imap/api/imap-folder-session.vala | 15 ++++++++++++- .../imap/response/imap-status-data.vala | 21 ++++++++++++++++--- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/src/engine/imap/api/imap-folder-session.vala b/src/engine/imap/api/imap-folder-session.vala index d1450755..5b3953c7 100644 --- a/src/engine/imap/api/imap-folder-session.vala +++ b/src/engine/imap/api/imap-folder-session.vala @@ -248,7 +248,20 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject { break; case ResponseCodeType.UIDNEXT: - this.folder.properties.uid_next = response_code.get_uid_next(); + try { + this.folder.properties.uid_next = response_code.get_uid_next(); + } catch (ImapError.INVALID err) { + // Some mail servers e.g hMailServer and + // whatever is used by home.pl (dovecot?) + // sends UIDNEXT 0. Just ignore these since + // there nothing else that can be done. See + // GNOME/geary#711 + if (response_code.get_as_string(1).as_int64() == 0) { + warning("Ignoring bad UIDNEXT 0 from server"); + } else { + throw err; + } + } break; case ResponseCodeType.UIDVALIDITY: diff --git a/src/engine/imap/response/imap-status-data.vala b/src/engine/imap/response/imap-status-data.vala index 9c1786e9..40d897f9 100644 --- a/src/engine/imap/response/imap-status-data.vala +++ b/src/engine/imap/response/imap-status-data.vala @@ -101,7 +101,20 @@ public class Geary.Imap.StatusData : Object { break; case StatusDataType.UIDNEXT: - uid_next = new UID.checked(valuep.as_int64()); + try { + uid_next = new UID.checked(valuep.as_int64()); + } catch (ImapError.INVALID err) { + // Some mail servers e.g hMailServer and + // whatever is used by home.pl (dovecot?) + // sends UIDNEXT 0. Just ignore these + // since there nothing else that can be + // done. See GNOME/geary#711 + if (valuep.as_int64() == 0) { + warning("Ignoring bad UIDNEXT 0 from server"); + } else { + throw err; + } + } break; case StatusDataType.UIDVALIDITY: @@ -118,8 +131,10 @@ public class Geary.Imap.StatusData : Object { break; } } catch (ImapError ierr) { - message("Bad value at %d/%d in STATUS response \"%s\": %s", ctr, ctr + 1, - server_data.to_string(), ierr.message); + warning( + "Bad value at %d/%d in STATUS response \"%s\": %s", + ctr, ctr + 1, server_data.to_string(), ierr.message + ); } }