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

restore lenient libmagic decoding #1375

Merged
merged 1 commit into from
Feb 7, 2019
Merged

Conversation

mjg
Copy link
Contributor

@mjg mjg commented Feb 4, 2019

176cffc ("refactor alot.db.utils.remove_cte", 2018-12-04) created a few
problems with 8bit quoted-printable e-mails, see #1291 #1360.

This commit restores the old libmagic fallback which did not cause this
problem.

(Maybe just goood as a 0.8 hotfix.)

176cffc ("refactor alot.db.utils.remove_cte", 2018-12-04) created a few
problems with 8bit quoted-printable e-mails, see pazz#1291 pazz#1360.

This commit restores the old libmagic fallback which did not cause this
problem.
@mjg mjg force-pushed the restore-libmagic-fallback branch from b06210a to dae40ed Compare February 4, 2019 17:15
@pazz pazz requested a review from lucc February 4, 2019 17:23
@pazz
Copy link
Owner

pazz commented Feb 4, 2019

Sorry I don't have time to review this properly.
Can you just confirm that this indeed fixes the issues in #1291 and #1360 and that we have test cases witnessing this?

@josch
Copy link
Contributor

josch commented Feb 4, 2019

I just backported the change to version 0.8 that we are about to upload to Debian and my results are:

@pazz
Copy link
Owner

pazz commented Feb 4, 2019

Thanks for this josh and mjg!

@lucc: what do you think?
Would this justify a minor release?

@mjg
Copy link
Contributor Author

mjg commented Feb 5, 2019

The force-pushed change shows that with this commit, the expected failure is turned into success.

What I don't know is whether the behavour change introduced in commit #176cffc was intentional, based on a concrete example, or just on the rationale "if there are decode errors then ignore undecodable bytes", which seems reasonable. I'm wondering: Is the message from the test case RFC compliant? It certainly is in a very common format. It is exactly this type that decode() fails to decode but on which libmagic succeeds. Maybe we can trace what libmagic actually does/guesses to understand what is going wrong? Maybe just a different CTE that we need to pass to decode()?

@dcbaker
Copy link
Collaborator

dcbaker commented Feb 6, 2019

Libmagic actually does some sort of analysis to figure out what the content encoding is, the remove_cte function trusts that a) the payload is truthful in it's encoding or b) that it does what the RFC requires and is 7bit if it doesn't declare an encoding.

FWIW, I think this is the right thing to do; try to trust the payload, if it lied/is wrong then fall back to libmagic.

Or, bump the version to 3.6 which IIRC can actually do this correctly.

Since this is blocking debian's adoption of 0.8, I think there should be a 0.8.1 release.

@pazz
Copy link
Owner

pazz commented Feb 6, 2019

@dcbaker: by "bump the version" you mean have alot eepend on python \ge 3.6 ?

@jljusten
Copy link
Contributor

jljusten commented Feb 7, 2019

@pazz, @dcbaker: I believe we are seeing this with python 3.7 on debian.

@pazz pazz merged commit 61b1c65 into pazz:master Feb 7, 2019
@lucc
Copy link
Collaborator

lucc commented Feb 7, 2019

@pazz sorry I'm late. I also vote for 0.8.1

@pazz
Copy link
Owner

pazz commented Feb 13, 2019

OK, I've just tagged this bugfix release (but won't announce it on the notmuch list)

@jljusten
Copy link
Contributor

jljusten commented Feb 14, 2019

@pazz: I think maybe you didn't change alot/__init__.py for the version before tagging, so the tarball content doesn't match the v0.8.1 tag. ?

@pazz
Copy link
Owner

pazz commented Feb 14, 2019

@jljusten: thanks and sorry: I only pushed the tags, not the last commit. It should be fixed now.

@mjg mjg deleted the restore-libmagic-fallback branch March 1, 2019 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants