Session::{fetch, uid_fetch}() fails to read full response and corrupts session

[tp971]

I'm not entirely sure what the issue is, however I have tried downloading a mailbox with thousands of mails and this occured: on a very big message, the result of session.uid_fetch(...).await?.try_next().await? returned None and the next request broke the connection with the error: io: inner stream closed. I did some further inspection with RUST_LOG=trace and found out that on every message I try to fetch which causes this error, the following happens:

[2022-12-17T16:05:25Z TRACE async_imap::imap_stream] decode: input: Ok("* 343 FETCH (INTERNALDATE ... some very long data and an HTML email ... </body>\r\n</html>\r\n UID ")
[2022-12-17T16:05:25Z TRACE async_imap::imap_stream] decode: incomplete data, need minimum 1 bytes

After that, it just return None and the next request returns an error. There seems to be an issue whenever the current input buffer ends with " UID ", because the only messages that ended with " UID " were the ones that caused this issue.

I'm seeing this too, but for non-UID fetches. I think the state corruption happens if you pass an empty sequence-list to fetch:

session.fetch("", "RFC822")...

... which can happen after a session.search(..).join(",") that yields no result.

I think async-imap needs a sanity check for empty strings passed to the search functions.

Edit: yep, here's the bug:

https://github.com/async-email/async-imap/blob/85ff7a3d9d71a3715354fabf2fc1a8d047b5710e/src/client.rs#L552-L556

If uid_set=="" the command sent will be corrupted.

This API really ought to be recast using richer types than using AsRef<str> to represent lists of sequence numbers. Details of the protocol parsing (like the fact that , is the separator and empty sequences won't parse properly) leak through the API... Why wasn't IntoIter<Seq> used here?

[tp971]

It has nothing to do with the sequence list. I managed to reproduce the bug here: https://github.com/tp971/async-imap-bug-fetch.

It has nothing to do with the sequence list.

How did you reach that conclusion?

[tp971]

Because in my case, the issue occurred without an empty sequence list. Sending an empty sequence list might possibly introduce a state corruption too, in that case this would be another issue, however in my case it happened with a non-empty sequence-list. If you look at my link above, it also has nothing to do with what is sent, rather than with what the library receives from the server.

[link2xt]

I have traced this down to setting of closed flag here:
https://github.com/async-email/async-imap/blob/721e519541414a5ac103f9251be77c052c86aa0b/src/imap_stream.rs#L307

If this is commented out, the test finishes successfully. Apparently this incorrectly detects an EOF when in fact it is just a temporarily empty buffer.

With the current code next .encode() emits a BrokenPipe error when attempting to write A0016 FETCH, which is wrong because even if read side is really closed it does not mean we cannot write another request. As long as the underlying stream allows us to write into it without errors, we can do it. I made a fix for this logic at #73.

Fix for detecting EOF is not ready yet.

[link2xt]

Made a fix #74

[link2xt]

The original bug is not completely fixed yet, I opened #78 for a follow-up.

[link2xt]

I pushed a regression test for this bug to 7c0e244