iconv being called with UTF8//IGNORE breaks on systems using musl

[te-online]

Expected behavior

Mail app loads inboxes of mail accounts

Actual behavior

Mail app shows "Could not open inbox" for every mail account

Description

This is an unfortunate "revival" of the bug fixed here: #309
Only this time it's in https://github.com/nextcloud/mail/blob/master/lib/Address.php#L82

Happy to help with a PR, but since it's such a small change it might be faster for an active maintainer to do it 😊 (because I don't know if I have everything set up in regards to commit signing and such...)

Mail app

Mail app version: 1.7.2

Mailserver or service: n/a

Server configuration

Operating system: Alpine

Web server: Nginx

Database: MariaDb

PHP version: 7.4.14

Nextcloud Version: 20.0.4

Client configuration

Browser: n/a

Operating system: n/a

[ChristophWurst]

Erm … we had a ticket for this #3999 but Github gives me a 404 😮

[ChristophWurst]

This is an unfortunate "revival" of the bug fixed here: #309

To be honest I'm not sure how much of a "fix" that was. We have the issue now that we store data from IMAP in the database and database don't like non-utf8 strings. That is why we do this filtering.

[te-online]

To be honest I'm not sure how much of a "fix" that was. We have the issue now that we store data from IMAP in the database and database don't like non-utf8 strings. That is why we do this filtering.

Well, for me a good indicator is that it generally works when passing UTF-8 while it doesn't work at all when passing UTF-8//IGNORE 😉 But you might have a point there ...

[sebarnd]

Same Problem here. The iconv("UTF-8", "UTF-8//IGNORE", $email); call seems to be in getEmail() in lib/Address.php

[PHP] Error: Error: iconv(): Wrong charset, conversion from `UTF-8' to `UTF-8//IGNORE' is not allowed at /var/www/html/custom_apps/mail/lib/Address.php#82 at <<closure>>

0. <<closure>>
   OC\Log\ErrorHandler::onError(8, "iconv(): Wrong  ... d", "/var/www/html/c ... p", 82, {email: "sebasti ... "})
1. /var/www/html/custom_apps/mail/lib/Address.php line 82
   iconv("UTF-8", "UTF-8//IGNORE", "++++++")
2. /var/www/html/custom_apps/mail/lib/Db/MessageMapper.php line 295
   OCA\Mail\Address->getEmail()
3. /var/www/html/custom_apps/mail/lib/Service/Sync/ImapToDbSynchronizer.php line 287
   OCA\Mail\Db\MessageMapper->insertBulk(OCA\Mail\Db\Message {id: null}, OCA\Mail\Db\Message {id: null})
4. /var/www/html/custom_apps/mail/lib/Service/Sync/ImapToDbSynchronizer.php line 215
   OCA\Mail\Service\Sync\ImapToDbSynchronizer->runInitialSync(OCA\Mail\Account {}, OCA\Mail\Db\Mailbox {id: 5}, OC\AppFramework\ScopedPsrLogger {})
5. /var/www/html/custom_apps/mail/lib/Service/Sync/ImapToDbSynchronizer.php line 128
   OCA\Mail\Service\Sync\ImapToDbSynchronizer->sync(OCA\Mail\Account {}, OCA\Mail\Db\Mailbox {id: 5}, OC\AppFramework\ScopedPsrLogger {}, 42, null, false, true)
6. /var/www/html/custom_apps/mail/lib/BackgroundJob/SyncJob.php line 92
   OCA\Mail\Service\Sync\ImapToDbSynchronizer->syncAccount(OCA\Mail\Account {}, OC\AppFramework\ScopedPsrLogger {})
7. /var/www/html/lib/public/BackgroundJob/Job.php line 80
   OCA\Mail\BackgroundJob\SyncJob->run({accountId: 1})
8. /var/www/html/lib/public/BackgroundJob/TimedJob.php line 61
   OCP\BackgroundJob\Job->execute(OC\BackgroundJob\JobList {}, OC\Log {})
9. /var/www/html/cron.php line 127
   OCP\BackgroundJob\TimedJob->execute(OC\BackgroundJob\JobList {}, OC\Log {})

at 2021-01-13T15:00:04+00:00

[HannesJo0139]

As far as I know this only happens on Alpine 3.12 and therefor might be an upstream issue. A script can be found here linuxserver/docker-nextcloud#165 that updates the iconv package and prevents that error. There was also a pull request that is unfortunately gone pull#166 (404) where they discussed that and the linuxserver.io/nextcloud devs stated that it should be solved with Alpine 3.13, which is about to be released shortly.

[ChristophWurst]

There was also a pull request that is unfortunately gone pull#166 (404)

Ok what is going on?! Our ticket #3999 disappeared as well. It was about this exact issue in Alpine Linux.

[stavros-k]

As far as I know this only happens on Alpine 3.12 and therefor might be an upstream issue. A script can be found here linuxserver/docker-nextcloud#165 that updates the iconv package and prevents that error. There was also a pull request that is unfortunately gone pull#166 (404) where they discussed that and the linuxserver.io/nextcloud devs stated that it should be solved with Alpine 3.13, which is about to be released shortly.

Sadly 3.13 did not fix the issue. But the script stills works.

[HannesJo0139]

As far as I know this only happens on Alpine 3.12 and therefor might be an upstream issue. A script can be found here linuxserver/docker-nextcloud#165 that updates the iconv package and prevents that error. There was also a pull request that is unfortunately gone pull#166 (404) where they discussed that and the linuxserver.io/nextcloud devs stated that it should be solved with Alpine 3.13, which is about to be released shortly.

Sadly 3.13 did not fix the issue. But the script stills works.

According to linuxserver/docker-nextcloud#165 you just have to re-add your accounts, then it whould work without script.

[stavros-k]

According to linuxserver/docker-nextcloud#165 you just have to re-add your accounts, then it whould work without script.

Confirming. It works now

[HannesJo0139]

For me too, so I guess this Issue can be closed

[te-online]

just have to re-add your accounts

I might say that this is not really a solution in my view 😢
We need some sort of migration for old user accounts.
But I'm not able to write another patch than what I already suggested...

[HannesJo0139]

@te-online Yeah I'm also wondering why that is necessary. Maybe @ChristophWurst can say sth about that?

[ChristophWurst]

I don't get why you have to re-add the account?! after the fix the sync will continue where it previously stopped, no?

[canatella]

By the way, this fixes only for docker user. I use an LXC container, I updated the mail app and just got stuck with it. I explained everything there: #3908 (comment) and #3908 (comment) . Is there a reason to keep that //IGNORE?

[ChristophWurst]

Is there a reason to keep that //IGNORE?

yes, to continue with the valid parts of the string. What else would you suggest to do in the error case?

[canatella]

Is there a reason to keep that //IGNORE?

yes, to continue with the valid parts of the string. What else would you suggest to do in the error case?

Well continuing with the valid parts doesn't make much sense to me. We receive an address, part of which is garbage
and we assume that the non garbage stuff is somehow okay to be used ? If the email is invalid, I suggest we don't try to repair it but raise an error. If we receive an invalid mail address with bad unicode characters, the most obvious reason for that would be some hacker trying to push in some bad payload. And if it was something else like a browser bug, then how can we trust any other part of the address to be valid ? Removing the garbage in the address will probably corrupt the adjacent characters too so you end up with a wrong address anyway which would be an actual bonus for someone trying to hack the system.

It all comes down to this: either the address is valid and we accept it, or it is not and we raise an exception instead of trying to fix it with the //IGNORE tag. Removing invalid bytes in it won't fix it so just removing the //IGNORE is the right fix. At least it seems to me ;)

[HannesJo0139]

I think you've got a point here but one should look at this in a differentiated way. Replacing this //IGNORE thing by a checkup as you described may be an improvement here that would additionally workaround the issue. But the issue itself is still upstream in systems where iconv does not work properly. Removing //IGNORE would work here but I disagree it being the right fix, as the upstream issue persists and is most likely going to cause trouble elsewhere.

[canatella]

I think you've got a point here but one should look at this in a differentiated way. Replacing this //IGNORE thing by a checkup as you described may be an improvement here that would additionally workaround the issue. But the issue itself is still upstream in systems where iconv does not work properly. Removing //IGNORE would work here but I disagree it being the right fix, as the upstream issue persists and is most likely going to cause trouble elsewhere.

No there are two issues. Code should not attempt to fix an email address by itself. You could as well try to fix [email protected] to [email protected] automatically, does that makes more sense ? The email is not valid, there is nothing you can do.

And about the upstream, like explained in my other comment, the musl implementation of iconv is also right. Iconv is a POSIX standard, defined here: https://pubs.opengroup.org/onlinepubs/9699919799/ where there is no mention of //IGNORE which is a gnu extension as mentioned in the gnu man page. Which means that it also won't work on a mac server (I know, who would right :) ) or any other system not using gnu iconv.

But like I said this second issue is not the problem. If input is invalid, you should not try to fix it. Could you give me an example of why you could get a badly encoded address and how automatically fixing it would be better than just signaling an error ?

[HannesJo0139]

Oh that clears things up for me. I totally agree then. Thanks!

[canatella]

No thank you all for doing all this work and taking the time to listen to me =)

[HannesJo0139]

@te-online @ChristophWurst So I have recovered a backup in order to re-check that "you must re-add your accounts" thing and can say it is not necessary. Maybe it was kind of a misunderstanding or so.

@ChristophWurst Nevertheless, as @canatella explained very well, using //IGNORE to "fix" invalid mail addresses seems to be a bad idea anyway. Invalid mail addresses should simply throw an error. Since LXC containers are still affected, a quick fix would be really nice. ;-)

[snevas]

Today the new images were pushed with alpine 3.13, so I tried again to switch from the main fpm image to the alpine one.
As I read that some say the account needs to be re-added, I've also tried using a fresh install with the mail app.

In all situations I still get the MailboxLockedError. Whenever I switch back to main fpm, the messages start to load.

[ChristophWurst]

In all situations I still get the MailboxLockedError

A locked mailbox is the symptom of a previous sync error. Check your logs for the actual cause.

[snevas]

There was no previous sync action. This is directly after adding the mailbox
See attached console log: console-export-2021-2-10_16-17-6.txt

[AlterDepp]

Problem still exists with dockerhub:nextloud 20.0.7-fpm-alpine with php:7.4-fpm-alpine3.13

[almereyda]

@AlterDepp You could try building your app container from the Dockerfile. With the snippet in nextcloud/docker#1299 (comment) it would then read somewhat like:

FROM nextcloud:fpm-alpine
RUN set -ex; \
 \
 apk add --no-cache \
   gnu-libiconv \
   php7-iconv \
 ;
ENV LD_PRELOAD=/usr/lib/preloadable_libiconv.so

[J0WI]

We use the PHP Docker image. php7-iconv pulls the PHP packages from the Alpine repo.
Upstream removed preloadable_libiconv.so completely.

See also:
docker-library/php#1121
https://gitlab.alpinelinux.org/alpine/aports/-/issues/12328
https://git.savannah.gnu.org/gitweb/?p=libiconv.git;a=commit;h=f38356d4414c023fcfbe34934d60c08ee94d5b8c

[ChristophWurst]

so what does that mean for Mail? are we doomed?

[canatella]

@ChristophWurst : nope, like I explained before, you could just remove the "//IGNORE" and still be as robust and secure. Please read my above comment if you don't see why.

[ChristophWurst]

in that case #4453 should help.

[HannesJo0139]

I'm still wondering about the logic behind using valid parts of a malformed e-mail address. After //IGNORE charset maybe valid again, but the address is still invalid. In fact //IGNORE means ignoring a fatal error. The pull request does not address this logic issue.

[ChristophWurst]

that is try. we just try to recover from the issue. what would you suggest instead? we had the "solution" where the app just fails hard on such data. this wasn't ideal either.

[HannesJo0139]

An invalid charset of email address means that the address itself is invalid as well. The affected email is not going to reach it's destination. A hard failing app is of course a bad thing. But an app that tries to ignore the error is not better at all.

As mentioned above //IGNORE statement is not even a part of the Iconv POSIX standard. Doing non-standardized things is always critical and error-prone. That is why we are here.

If you want to keep the logic of ignoring the issue, just do it without using //IGNORE. Better way would be to soft fail. Abort the action and show an error message so that the user is informed that something went wrong.

[ChristophWurst]

wait. the email value sanitization is used for the messages that we read from IMAP, not new message sent by the user.

[HannesJo0139]

Ah I see. Sorry for the confusion. But there is still the problem of using non-standard code that fails in some environments. The pull request just avoids the error for well-formed addresses. It does not solve this issue. It just makes the issue happen not so often.

[ChristophWurst]

That's true, @HannesJo0139. It avoid the issue if possible but in extreme cases it will still show.

[AlexCloudDev]

Hello,
i tried to read and understand the whole conversation, but i don't know if i missed something.

I can confirm that this error still persists.
I'm on the latest nextcloud fpm-alpine docker image, alpine version inside the container is 3.13.2.
I'm also on the latest mail app 1.8.3.

But as soon as i or an user tries to open the mail interface, the nextcloud log is spammed with
Error: iconv(): Wrong charset, conversion from UTF-8' to UTF-8//IGNORE' is not allowed at /var/www/html/custom_apps/mail/lib/Model/IMAPMessage.php#276
and
OCA\Mail\Exception\ServiceException: Sync failed for 90:INBOX: Return value of OCA\Mail\Model\IMAPMessage::getSubject() must be of the type string, bool returned

So how can i fix this?
Will this be fixed in a future update?

I remember, this problem persists since the nc 20 release, when i'm right.

Thank your,
regards

[HannesJo0139]

@AlexCloudDev The patch is likely coming with 1.8.4. You can also apply it manually (https://patch-diff.githubusercontent.com/raw/nextcloud/mail/pull/4483.patch). Unfortunately it won't solve all cases where the issue occurs. In rare cases of mal-formed encoding it is still going to occur. But the mail app should be usable again.

[AlexCloudDev]

@AlexCloudDev The patch is likely coming with 1.8.4. You can also apply it manually (https://patch-diff.githubusercontent.com/raw/nextcloud/mail/pull/4483.patch). Unfortunately it won't solve all cases where the issue occurs. In rare cases of mal-formed encoding it is still going to occur. But the mail app should be usable again.

Okay, thx, sound good.
Any ETA for 1.8.4?

regards

[AlterDepp]

Version 1.9.1 works for me, thanks

[AlexCloudDev]

Version 1.9.1 works for me, too.

Thank you very much for your time and effort.