Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor tag assertion to FatalError check & Fix unexpected increase of tag #231

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use std::str;
use std::sync::mpsc;

use super::authenticator::Authenticator;
use super::error::{Bad, Bye, Error, No, ParseError, Result, ValidateError};
use super::error::{Bad, Bye, Error, No, ParseError, Result, TagCorrupted, ValidateError};
use super::extensions;
use super::parse::*;
use super::types::*;
Expand Down Expand Up @@ -1345,7 +1345,11 @@ impl<T: Read + Write> Connection<T> {

fn run_command(&mut self, untagged_command: &str) -> Result<()> {
let command = self.create_command(untagged_command);
self.write_line(command.into_bytes().as_slice())
let result = self.write_line(command.into_bytes().as_slice());
if result.is_err() {
self.tag -= 1; // rollback tag increase in create_command()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the discussion in #229, I'm okay with this change, though I think we should also add a skip_tag method to Client to allow manually incrementing the tag.

}
result
}

fn run_command_and_read_response(&mut self, untagged_command: &str) -> Result<Vec<u8>> {
Expand Down Expand Up @@ -1397,7 +1401,17 @@ impl<T: Read + Write> Connection<T> {
..
},
)) => {
assert_eq!(tag.as_bytes(), match_tag.as_bytes());
// check if tag matches
if tag.as_bytes() != match_tag.as_bytes() {
let expect = self.tag;
let actual = tag
.0
.trim_start_matches(TAG_PREFIX)
.parse::<u32>()
.unwrap_or_default();
break Err(Error::TagCorrupted(TagCorrupted { expect, actual }));
}

Some(match status {
Status::Bad | Status::No | Status::Bye => Err((
status,
Expand Down
35 changes: 35 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ pub enum Error {
/// In response to a STATUS command, the server sent OK without actually sending any STATUS
/// responses first.
MissingStatusResponse,
/// Tag mismatch between client and server. New session must be created.
TagCorrupted(TagCorrupted),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name TagCorrupted is misleading. A "corrupted" tag feels more like if a tag isn't a number for example, or has somehow otherwise been modified in-flight. What this represents (as the doc string indicates) is a TagMismatch or Desynchronized, so let's call it one of those.

}

impl From<IoError> for Error {
Expand Down Expand Up @@ -170,6 +172,7 @@ impl fmt::Display for Error {
Error::Append => f.write_str("Could not append mail to mailbox"),
Error::Unexpected(ref r) => write!(f, "Unexpected Response: {:?}", r),
Error::MissingStatusResponse => write!(f, "Missing STATUS Response"),
Error::TagCorrupted(ref data) => write!(f, "Mismatched Tag: {:?}", data),
}
}
}
Expand All @@ -194,6 +197,7 @@ impl StdError for Error {
Error::Append => "Could not append mail to mailbox",
Error::Unexpected(_) => "Unexpected Response",
Error::MissingStatusResponse => "Missing STATUS Response",
Error::TagCorrupted(ref e) => e.description(),
}
}

Expand All @@ -207,6 +211,7 @@ impl StdError for Error {
#[cfg(feature = "native-tls")]
Error::TlsHandshake(ref e) => Some(e),
Error::Parse(ParseError::DataNotUtf8(_, ref e)) => Some(e),
Error::TagCorrupted(ref e) => Some(e),
_ => None,
}
}
Expand Down Expand Up @@ -285,6 +290,36 @@ impl StdError for ValidateError {
}
}

/// Tag was corrupted during session.
#[derive(Debug)]
#[non_exhaustive]
pub struct TagCorrupted {
/// Expected tag number
pub(crate) expect: u32,
/// Actual tag number, 0 if parse failed
pub(crate) actual: u32,
}

impl fmt::Display for TagCorrupted {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(
f,
"Expected tag number is {}, actual {}",
self.expect, self.actual
)
}
}

impl StdError for TagCorrupted {
fn description(&self) -> &str {
"Tag is corrupted, session is in an inconsistent state"
}

fn cause(&self) -> Option<&dyn StdError> {
None
}
Comment on lines +314 to +320
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can skip both of these fns — they're not required and their default implementations will do the right thing.

}

#[cfg(test)]
mod tests {
use super::*;
Expand Down