Skip to content

"base64 decode assumes input lines are in multiples of 4 #96" #99

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

Closed
wants to merge 1 commit into from

Conversation

vandys
Copy link

@vandys vandys commented Feb 22, 2023

Convert base64 implementation of stateless to stateful, so you can feed in successive lines and get back the original binary. The previous stateless implementation assumes that the input has no residual after processing a single line, which is not a semantic guaranteed by the base64-in-email standards (and, in fact, was found while processing exports from Tutanota).

Convert base64 implementation of stateless to stateful, so you can
feed in successive lines and get back the original binary.  The
previous stateless implementation assumes that the input has
no residual after processing a single line, which is not a semantic
guaranteed by the base64-in-email standards (and, in fact, was
found while processing exports from Tutanota).
@jkbzh
Copy link
Contributor

jkbzh commented May 15, 2023

@vandys I'm reviewing your code, you don't need to update anything.

According to man, bzero has been deprecated in favor of memset, so I reverted that specific change and removed the strings.h include.

Seeing that base64 is so common in messages and headers and that hypermail is not processing message parts or headers in parallel, I wonder if it's worth it to allocate and free the state structure each time we get a new part and if it wouldn't be better to just use a variable and fixed memory instead of a pointer and allocated memory.

What do you think?

@vandys
Copy link
Author

vandys commented May 15, 2023

Yes, bcopy/bzero are bad old habits from my old BSD days!
If you keep a single state, I'd still recommend being sure to reset it at the points where its use starts and ends. I'm sure at some point a malformed base64 will try to bleed from one attachment to another.
OTOH, it's hard to imagine that the average processing of a base64 attachment not entirely swamping the small overhead of allocation and free. Either seems fine to me!

free(data); /* this was allocatd by mdecodeQP() */
}
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

as you saw, lines 3435-3440 cause a sigsev in your base64 changes because it frees the memory right after allocating it.

That free there is only meant for the QP decoder. It decodes everything in the single call to the decoder and the data variable is freed afterwards.

No need to change anything

@jkbzh
Copy link
Contributor

jkbzh commented May 16, 2023

@vandys Thank you for your excellent patch. All tests I wrote for it worked well.

I edited it a bit (function and state structure names, fields, ...) and manually merged it to the 3.0 branch. 2bd8ed5

I did a small fix. The place where you were freeing the state structure only worked in some cases. I added that release code in the two places where it should have gone.

Other than those small changes, I didn't need to change anything.

@jkbzh jkbzh closed this May 16, 2023
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.

3 participants