[geary/wip/781488-aliyun-greeting-error: 6/6] Ensure syntax errors are always reported by the deserialiser.
- From: Michael Gratton <mjog src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [geary/wip/781488-aliyun-greeting-error: 6/6] Ensure syntax errors are always reported by the deserialiser.
- Date: Mon, 30 Oct 2017 06:22:11 +0000 (UTC)
commit 2b58014c82089ec080d32db158e304acf3c64f5e
Author: Michael James Gratton <mike vee net>
Date: Mon Oct 30 16:58:44 2017 +1100
Ensure syntax errors are always reported by the deserialiser.
Add unit tests.
* src/engine/imap/transport/imap-deserializer.vala (Deserialiser): Add a
new EOL even handler to the FAILED state that is the one place where
the deserialize_failure() signal is fired and that any existing params
are reset.
(Deserialiser::push_line): Remove signal calls and unused return
value. Don't use EOS to detect NUL to avoid some confusion. Ensure an
EOL event is always fired so that the FAILED EOF handler is always run.
(Deserialiser::push_data): Remove signal calls and unused return value.
(Deserialiser::flush_params): Don't bother returning a state, since we
now always know what the next one should be when calling it. Only
trigger the parameters_ready() signal if there is a valid
parameter. Make logging consistent with the rest of the class.
(Deserialiser::reset_params): New common method for resetting any
existing deserialised message.
(Deserialiser::on_literal_char): Minor tidy up.
(Deserialiser::on_eos): Ensure that any existing param is sent on EOS
so that any BYE+EOS sent by the server is picked up.
src/engine/imap/transport/imap-deserializer.vala | 157 ++++++++++----------
.../imap/transport/imap-deserializer-test.vala | 60 ++++++++
2 files changed, 139 insertions(+), 78 deletions(-)
---
diff --git a/src/engine/imap/transport/imap-deserializer.vala
b/src/engine/imap/transport/imap-deserializer.vala
index 525721f..7c24c9b 100644
--- a/src/engine/imap/transport/imap-deserializer.vala
+++ b/src/engine/imap/transport/imap-deserializer.vala
@@ -195,10 +195,11 @@ public class Geary.Imap.Deserializer : BaseObject {
new Geary.State.Mapping(State.LITERAL_DATA, Event.DATA, on_literal_data),
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.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),
-
+
new Geary.State.Mapping(State.CLOSED, Event.EOS, Geary.State.nop),
new Geary.State.Mapping(State.CLOSED, Event.ERROR, Geary.State.nop)
};
@@ -314,15 +315,12 @@ public class Geary.Imap.Deserializer : BaseObject {
return;
}
-
- Logging.debug(Logging.Flag.DESERIALIZER, "[%s] line %s", to_string(), line);
-
+
+ Logging.debug(Logging.Flag.DESERIALIZER, "[%s] line: %s", to_string(), line);
bytes_received(bytes_read);
-
push_line(line, bytes_read);
} catch (Error err) {
push_error(err);
-
return;
}
@@ -343,12 +341,11 @@ public class Geary.Imap.Deserializer : BaseObject {
}
Logging.debug(Logging.Flag.DESERIALIZER, "[%s] block %lub", to_string(), bytes_read);
-
bytes_received(bytes_read);
-
+
// adjust the current buffer's size to the amount that was actually read in
block_buffer.trim(current_buffer, bytes_read);
-
+
push_data(bytes_read);
} catch (Error err) {
push_error(err);
@@ -358,51 +355,37 @@ public class Geary.Imap.Deserializer : BaseObject {
next_deserialize_step();
}
-
+
// Push a line (without the CRLF!). Because DataInputStream reads to EOL but accepts all other
// characters, it's possible for NULs to be embedded in the string, so the count must be passed
// as well (and shouldn't be -1!)
- private Mode push_line(string line, size_t count) {
+ private void push_line(string line, size_t count) {
assert(get_mode() == Mode.LINE);
-
+
for (long ctr = 0; ctr < count; ctr++) {
char ch = line[ctr];
- if (ch == String.EOS) {
+ if (ch == '\0') {
// drop on the floor; IMAP spec does not allow NUL in lines at all, see
// http://tools.ietf.org/html/rfc3501#section-9 note 3.
- // This also catches the terminating NUL.
continue;
}
-
+
if (fsm.issue(Event.CHAR, &ch) == State.FAILED) {
- deserialize_failure();
-
- return Mode.FAILED;
+ // Don't try to continue processing, but continue to
+ // EOL regardless for cleanup.
+ break;
}
}
-
- if (fsm.issue(Event.EOL) == State.FAILED) {
- deserialize_failure();
-
- return Mode.FAILED;
- }
-
- return get_mode();
+
+ fsm.issue(Event.EOL);
}
-
+
// Push a block of literal data
- private Mode push_data(size_t bytes_read) {
+ private void push_data(size_t bytes_read) {
assert(get_mode() == Mode.BLOCK);
-
- if (fsm.issue(Event.DATA, &bytes_read) == State.FAILED) {
- deserialize_failure();
-
- return Mode.FAILED;
- }
-
- return get_mode();
+ fsm.issue(Event.DATA, &bytes_read);
}
-
+
// Push an EOS event
private void push_eos() {
fsm.issue(Event.EOS);
@@ -465,11 +448,7 @@ public class Geary.Imap.Deserializer : BaseObject {
current_string = null;
}
-
- private void clear_string_parameter() {
- current_string = null;
- }
-
+
private void save_literal_parameter() {
save_parameter(new LiteralParameter(block_buffer));
block_buffer = null;
@@ -502,28 +481,34 @@ public class Geary.Imap.Deserializer : BaseObject {
return State.START_PARAM;
}
-
- private State flush_params() {
- if (context != root) {
- warning("Unclosed list in parameters");
-
- return State.FAILED;
+
+ private void flush_params() {
+ bool okay = true;
+ if (this.context != this.root) {
+ Logging.debug(Logging.Flag.DESERIALIZER, "[%s] Unclosed list in parameters");
+ okay = false;
}
-
- if (!is_current_string_empty() || literal_length_remaining > 0) {
- warning("Unfinished parameter: string=%s literal remaining=%lu",
- (!is_current_string_empty()).to_string(), literal_length_remaining);
-
- return State.FAILED;
+
+ if (!is_current_string_empty() || this.literal_length_remaining > 0) {
+ Logging.debug(
+ Logging.Flag.DESERIALIZER,
+ "Unfinished parameter: string=%s literal remaining=%lu",
+ (!is_current_string_empty()).to_string(), this.literal_length_remaining
+ );
+ okay = false;
}
-
- RootParameters ready = root;
- root = new RootParameters();
- context = root;
-
- parameters_ready(ready);
-
- return State.TAG;
+
+ if (okay && this.root != null && root.size > 0) {
+ parameters_ready(this.root);
+ }
+
+ reset_params();
+ }
+
+ private void reset_params() {
+ this.context = this.root = new RootParameters();
+ this.current_string = null;
+ this.literal_length_remaining = 0;
}
//
@@ -672,16 +657,29 @@ public class Geary.Imap.Deserializer : BaseObject {
}
private uint on_eol(uint state, uint event, void *user) {
- return flush_params();
+ flush_params();
+ return State.TAG;
}
-
+
private uint on_atom_eol(uint state, uint event, void *user) {
// clean up final atom
save_string_parameter(false);
-
- return flush_params();
+ flush_params();
+ return State.TAG;
}
-
+
+ private uint on_failed_eol(uint state, uint event, void *user) {
+ // We don't go into the error state on syntax error, merely
+ // reset, cross our fingers and hope for the best. Maybe
+ // there's a better strategy though?
+ Logging.debug(
+ Logging.Flag.DESERIALIZER, "[%s] Syntax error, dropping", to_string()
+ );
+ deserialize_failure();
+ reset_params();
+ return State.TAG;
+ }
+
private uint on_quoted_char(uint state, uint event, void *user) {
char ch = *((char *) user);
@@ -765,16 +763,14 @@ public class Geary.Imap.Deserializer : BaseObject {
// empty literal treated as garbage
if (is_current_string_empty())
return State.FAILED;
-
- literal_length_remaining = (size_t) long.parse(current_string.str);
+
+ literal_length_remaining = (size_t) long.parse(this.current_string.str);
+ this.current_string = null;
if (literal_length_remaining < 0) {
warning("Negative literal data length %lu", literal_length_remaining);
-
return State.FAILED;
}
-
- clear_string_parameter();
-
+
return State.LITERAL_DATA_BEGIN;
}
@@ -804,17 +800,22 @@ public class Geary.Imap.Deserializer : BaseObject {
return State.START_PARAM;
}
-
+
private uint on_eos() {
debug("[%s] EOS", to_string());
-
+
+ // Per RFC 3501 7.1.5, we may get a EOS immediately after a
+ // BYE, so flush any message being processed.
+ // https://tools.ietf.org/html/rfc3501#section-7.1.5
+ flush_params();
+
// always signal as closed and notify subscribers
closed_semaphore.blind_notify();
eos();
-
+
return State.CLOSED;
}
-
+
private uint on_error(uint state, uint event, void *user, Object? object, Error? err) {
assert(err != null);
diff --git a/test/engine/imap/transport/imap-deserializer-test.vala
b/test/engine/imap/transport/imap-deserializer-test.vala
index a12f699..d8b1f1a 100644
--- a/test/engine/imap/transport/imap-deserializer-test.vala
+++ b/test/engine/imap/transport/imap-deserializer-test.vala
@@ -23,10 +23,17 @@ class Geary.Imap.DeserializerTest : Gee.TestCase {
add_test("test_cyrus_2_4_greeting", test_cyrus_2_4_greeting);
add_test("test_aliyun_greeting", test_aliyun_greeting);
+ add_test("test_invalid_atom_prefix", test_invalid_atom_prefix);
+
add_test("test_gmail_flags", test_gmail_flags);
add_test("test_gmail_permanent_flags", test_gmail_permanent_flags);
add_test("test_cyrus_flags", test_cyrus_flags);
+ add_test("test_runin_special_flag", test_runin_special_flag);
+ add_test("test_invalid_flag_prefix", test_invalid_flag_prefix);
+
+ add_test("test_instant_eos", test_instant_eos);
+ add_test("test_bye_eos", test_bye_eos);
}
public override void set_up() {
@@ -73,6 +80,15 @@ class Geary.Imap.DeserializerTest : Gee.TestCase {
assert(message.to_string() == parsed);
}
+ public void test_invalid_atom_prefix() {
+ string flags = """* OK %atom""";
+ this.stream.add_data(flags.data);
+ this.stream.add_data(EOL.data);
+
+ this.process.begin(Expect.DESER_FAIL, (obj, ret) => { async_complete(ret); });
+ this.process.end(async_result());
+ }
+
public void test_gmail_flags() {
string flags = """* FLAGS (\Answered \Flagged \Draft \Deleted \Seen $NotPhishing $Phishing)""";
this.stream.add_data(flags.data);
@@ -106,6 +122,50 @@ class Geary.Imap.DeserializerTest : Gee.TestCase {
assert(message.to_string() == flags);
}
+ public void test_runin_special_flag() {
+ // since we must terminate a special flag upon receiving the
+ // '*', the following atom will be treated as a run-on but
+ // distinct atom.
+ string flags = """* OK \*atom""";
+ string expected = """* OK \* atom""";
+ this.stream.add_data(flags.data);
+ this.stream.add_data(EOL.data);
+
+ this.process.begin(Expect.MESSAGE, (obj, ret) => { async_complete(ret); });
+ RootParameters? message = this.process.end(async_result());
+
+ assert(message.to_string() == expected);
+ }
+
+ public void test_invalid_flag_prefix() {
+ string flags = """* OK \%atom""";
+ this.stream.add_data(flags.data);
+ this.stream.add_data(EOL.data);
+
+ this.process.begin(Expect.DESER_FAIL, (obj, ret) => { async_complete(ret); });
+ this.process.end(async_result());
+ }
+
+ public void test_instant_eos() {
+ this.process.begin(Expect.EOS, (obj, ret) => { async_complete(ret); });
+ this.process.end(async_result());
+ assert(this.deser.is_halted());
+ }
+
+ public void test_bye_eos() {
+ string bye = """* OK bye""";
+ this.stream.add_data(bye.data);
+
+ bool eos = false;
+ this.deser.eos.connect(() => { eos = true; });
+
+ this.process.begin(Expect.MESSAGE, (obj, ret) => { async_complete(ret); });
+ RootParameters? message = this.process.end(async_result());
+ assert(message.to_string() == bye);
+ assert(eos);
+ assert(this.deser.is_halted());
+ }
+
protected async RootParameters? process(Expect expected) {
RootParameters? message = null;
bool eos = false;
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
