When reading literal_data from mail body from IMAP, use byte count instead of string-length count.

[indridieinarsson]

Pullrequest

This is a first attempt to fix issue #1119.
I'm not very proficient in php, and not knowledgeable about imap, so for the love of all that is good and holy, review and test this properly.

Take a look at this chunk from hm-imap-base.php:

elseif ($line[$i] == '{') {
$end = mb_strpos($line, '}');
if ($end !== false) {
$literal_size = mb_substr($line, ($i + 1), ($end - $i - 1));
}
$lit_result = $this->read_literal($literal_size, $max, $current_size, $line_length);
$chunk = $lit_result[0];

If I understand this writing correctly: https://www.rfc-editor.org/rfc/rfc3501#section-4.3, the number in curly braces that is being read into $literal_size, is the number of bytes in the email body, which is then sent over the wire right away.
Therefore, all string-size calculations in read_literal() should use the strsize rather than mb_strsize command. My theory is that the issue #1119 is caused by this: the number of bytes read are underestimated since mb_strsize is smaller than strsize for real multibyte strings. We then end up trying to read strings from the server after the server has sent the entire message, causing the code to hang.

Issues

• Might possibly fix FastCGI timeout (Not docker related) #1119

Checklist

This touches the code that reads the email body, pretty central for an email client. This PR will change behaviour in cases when the body of the message contains multibyte characters (ÞþæÆöÖðÐáÁóÓ etc.).

[kambereBr]

Looks good! Fix the pipelines, and we’ll merge. We'll keep an eye on the changes. Thanks!

[indridieinarsson]

@kambereBr : I can't really figure out how to get the Selenium test to pass. Can't see that my code should have change what the test is checking. Is the test somehow broken? Or has a previous commit inadvertently broken it?

[indridieinarsson]

I installed the version in my feature-branch, with bunch of emails and 3 accounts. With the exception of an already reported issue, everything works as expected. In particular, the Selenium test that is failing is not reproducible in the browser.

[indridieinarsson]

Ok, I did some searching around. Seems the test was failing due to a race condition.

• We find an element and check its text.
• Then we refresh the message list, which also reloads the page, and in that process, the element we were looking for is nuked and rebuild.
• Then we go on to the next line to find the element again, but the reference is still pointing to the old element at this point, since the refresh is not yet finished, so we still get a reference to the old element.
• the old element is stale, and we get an error
  I added some code that waits for the element to be available and not stale. If the element is not available after a timeout of 10s, it should throw an error.

Someone might want to re-review this PR, as I fiddled with the tests.

[marclaporte]

This touches the code that reads the email body, pretty central for an email client. This PR will change behaviour in cases when the body of the message contains multibyte characters (ÞþæÆöÖðÐáÁóÓ etc.).

Well, I think it reverts some of the recent changes so it restores code to what is was for a long time.

Here are recent modification on this file:
https://github.com/cypht-org/cypht/commits/master/modules/imap/hm-imap-base.php

[marclaporte]

Here is the change: #1051

Could some of the other changes have unintendended negative consequences?

[kambereBr]

Here is the change: #1051

Could some of the other changes have unintendended negative consequences?

Probably yes, but it's hard to say for sure. We'll only know when we encounter issues in specific use cases.

[marclaporte]

Ok, we'll stay alert :-) #1224

[kroky]

@indridieinarsson thanks for the PR but since we have more problems than the mentioned issue with literal string lengths, I combined your code with other fixes and some unit tests to catch this issue in the future here: #1230
Note for the discussion about using 8-bit string functions vs multibyte ones: we prefer and should use the multibyte functions everywhere we deal with strings in Cypht. The only exception seems to be here in imap module where literal lengths is specified in bytes rather than characters. It is actually somewhat strange as the RFC mentions only 7-bit encoding or 8-bit ones but maybe some mail servers still use unencoded multibytes in those literals. Thus, we make sure that read_literal works on bytes and doesn't check individual bytes for valid character values.
The other problem is unfinished changes from the multibyte conversion where accessing a string via array offset is not actually working for multibyte strings - we should use mb_substr in those cases.