[jengelh]

With a finite line length, vmime produced headers that other parsers read back wrong. With an infinite one, encoded-words had no size limit.

• encoded-words stay within 75 chars, also without a line limit
• no doubled spaces around encoded-words, which follows up on d296c2d
• '"' and '' in display names are escaped, and names stay quoted when folded
• folds are never placed after '', and only at the last space of a run
• whole addresses move to the next line, with no trailing whitespace and one space before a folded
• 7-bit local parts are never encoded or folded
• message-id lists fold in place of the separator
• the list parser ignores '(', '<' and '"' inside quotes and comments

[vincent-richard]

is7bit() is also false for =? and for a trailing = (see countASCIIchars()), so a plain ASCII local part like bounce-dGVzdA== gets encoded, down to the SMTP envelope. Checking for printable ASCII fixes that. It also keeps ISO-2022-JP (ESC sequences) and CR/LF on the encoded path; otherwise ISO-2022-JP would be written as raw ESC bytes:

Suggested change

	if (utility::stringUtils::is7bit(buffer) &&
	buffer.find_first_of("\r\n") == string::npos) {
	// Not is7bit(), which is also false for "=?" and a trailing '='.
	// ESC (ISO-2022-JP), CR and LF are not printable.
	bool printable = true;
	for (size_t i = 0 ; printable && i < buffer.length() ; ++i) {
	printable = parserHelpers::isPrint(buffer[i]);
	}
	if (printable) {

[vincent-richard]

Test for the emailAddress.cpp fix, see below.

Suggested change

	VMIME_TEST(testGenerateLongLocalPart)
	VMIME_TEST(testGenerateLongLocalPart)
	VMIME_TEST(testGenerateASCIILocalPart)

[vincent-richard]

It fails without the is7bit() fix (=?utf-8?Q?bounce-dGVzdA=3D=3D?=@example.com).

Suggested change

	}
	}
	void testGenerateASCIILocalPart() {
	// Plain ASCII is not encoded, even with '=' (eg. VERP) or "=?"
	const char* locals[] = { "bounce-dGVzdA==", "abc=", "a=?b" };
	for (const char* local : locals) {
	vmime::emailAddress e(local, "example.com");
	VASSERT_EQ(local, std::string(local) + "@example.com", e.generate());
	}
	vmime::emailAddress back;
	back.parse(vmime::emailAddress("bounce-dGVzdA==", "example.com").generate());
	VASSERT_EQ("parse", "bounce-dGVzdA==", back.getLocalName().getBuffer());
	// 7-bit, but with escape sequences: still encoded
	const vmime::word jp("\x1b$B$3$s$K$A$O\x1b(B", vmime::charset("iso-2022-jp"));
	const std::string out =
	vmime::emailAddress(jp, vmime::word("example.com", vmime::charsets::US_ASCII)).generate();
	VASSERT("iso-2022-jp: " + out, out.compare(0, 16, "=?iso-2022-jp?B?") == 0);
	VASSERT_EQ("no ESC", std::string::npos, out.find('\x1b'));
	}

[vincent-richard]

This replaces the UTF-8 locale picked by setUTF8Locale() (tests/testUtils.cpp) with the environment one: "C" on the Cygwin runner, where LANG is unset. tearDown() resets it to "C" anyway. That's the Cygwin failure, and LC_ALL=C ./parser_textTest reproduces it on Linux (testWordGenerateQuote). No test needs it:

Suggested change

	void setUp() {
	// Set the global C and C++ locale to the user-configured locale.
	// The locale should use UTF-8 encoding for these tests to run successfully.
	try {
	std::locale::global(std::locale(""));
	} catch (std::exception &) {
	std::setlocale(LC_ALL, "");
	}
	}
	void tearDown() {
	// Restore default locale
	std::locale::global(std::locale("C"));
	}

[vincent-richard]

Case 2 now gives the same output as case 1, so it doesn't test much anymore. At 19, aaa=C3 would fit but not the whole character, and 22 brings back the old expectation:

Suggested change

	VASSERT_EQ(
	"2",
	"=?utf-8?Q?aaa=C3=A9?==?utf-8?Q?zzz?=",
	"=?utf-8?Q?aaa?==?utf-8?Q?=C3=A9?==?utf-8?Q?zzz?=",
	cleanGeneratedWords(
	vmime::word("aaa\xc3\xa9zzz", vmime::charset("utf-8")).generate(17)
	)
	);
	// "aaa=C3" fits, but not the whole character
	VASSERT_EQ(
	"2",
	"=?utf-8?Q?aaa?==?utf-8?Q?=C3=A9?==?utf-8?Q?zzz?=",
	cleanGeneratedWords(
	vmime::word("aaa\xc3\xa9zzz", vmime::charset("utf-8")).generate(19)
	)
	);
	VASSERT_EQ(
	"3",
	"=?utf-8?Q?aaa=C3=A9?==?utf-8?Q?zzz?=",
	cleanGeneratedWords(
	vmime::word("aaa\xc3\xa9zzz", vmime::charset("utf-8")).generate(22)
	)
	);

[vincent-richard]

Nit: FoldPoint below is declared without an anonymous namespace, like most file-local types in the codebase (e.g. CharsetEncodingEntry in charset.cpp), so maybe the same here:

Suggested change

	namespace {
	enum QuoteResult {
	QUOTE_NOT_DONE, // nothing written, fold the text unquoted
	QUOTE_DONE,
	QUOTE_NEEDS_ENCODING // nothing written, a run does not fit on a line
	};
	}
	enum QuoteResult {
	QUOTE_NOT_DONE, // nothing written, fold the text unquoted
	QUOTE_DONE,
	QUOTE_NEEDS_ENCODING // nothing written, a run does not fit on a line
	};

[vincent-richard]

Oops, summary of my review got lost, here it is:

Thanks a lot for this, it fixes a bunch of long-standing folding issues!

I built it and ran the tests (each commit builds and passes the parser tests on its own, excepot for Cygwin, see below). I also threw a quick differential fuzzer at it: random text / mailbox / mailbox list, master vs. this branch, 20k iterations, same inputs on both sides. Results look great: no round-trip regressions, and over-long encoded-words, broken display-name round-trips and broken list round-trips all drop to zero.

I think 3 things need fixing before merging. I left a suggestion for each one inline, and tested them on top of your branch: all tests pass, also with LC_ALL=C.

1. Local parts ending with = are now encoded, including in the SMTP envelope:

   master:  MAIL FROM:<bounce-dGVzdA==@example.com>
   PR:      MAIL FROM:<=?us-ascii?Q?bounce-dGVzdA=3D=3D?=@example.com>
   

2. The Cygwin CI failure comes from the new setUp()/tearDown() in textTest.
3. testWordGenerateMultiBytes: case 2 is now identical to case 1.

The parameterTest and wordEncoderTest changes look good to me. In parameterTest, the first line is still 81 chars for a max of 80, but that's an old issue in parameter.cpp, which generates the value at column 0. Not for this PR.

Some questions

• No fold after \: when that's the only fold point, the line can grow well past the limit. E.g. "aaa…a C:\" + " änderungen änderungen" at column 20, max 78 → a 93-char line containing an encoded-word, which goes against RFC 2047's 76 limit, whereas a fold after \ is fine per RFC 5322. Which parsers did you see misbehave with this (same question for the space runs and tabs)? Depending on the answer, I'd rather keep the fold as a fallback.
• A first ASCII word that doesn't fit on the line is now encoded. E.g. a 70–74 char URL after Subject: becomes =?us-ascii?Q?https=3A//...?=, and tab-separated text gets encoded too. For tabs, folding as CRLF + TAB keeps the tab and is valid RFC 5322 — any reason to avoid it below 998?

Smaller stuff

• PREV_ENDS_WITH_BACKSLASH is defined twice (text.cpp, word.cpp) and passed through the public flags. A field in word::generatorState, next to lastCharIsSpace, would feel more natural. It's also based on the input buffer: a quoted word ending with \ actually ends with " in the output.
• mailboxGroup::generateImpl() still writes ", " before each mailbox, so the "whole addresses on the next line" logic doesn't apply inside groups.
• A tab at the start of an encoded word following a plain word still gets an extra space ("foo@\tä" comes back as "foo@ \tä"). Pre-existing, but close to the "no doubled spaces" fix.

Thanks again!

[grammmike]

Thanks for the thorough review and the fuzzing as well as the kind words.

I've updated the branch. each fix is squashed into the commit it belongs to, plus one new commit at the end. every commit builds and passes the parser tests, also with LC_ALL=C.

The three blockers:

1. local parts: taken as you suggested (printable check + your test), squashed into the local-part commit. bounce-dGVzdA==@example.com is no longer encoded.
2. Cygwin: setUp()/tearDown() are removed from textTest.
3. testWordGenerateMultiBytes: now uses 19 and 22, as you suggested.

Regarding your questions:

• fold after : you're right, a line past the 76 limit is worse. A fold after \ is now a fallback, used only when there's no other fold point. The order is: single space, end of a white-space run, tab, after . Your C:\ + " änderungen änderungen" example now folds after the \ and stays within 76. the parsers that made us avoid it were: some customer-submitted python and imap examples compared with Exchange which dealt with it apropriately. honestly, I don't have the exact scripts, just by the mail dumps, and Exchange (and dovecot which we also checked) "got it right".
• first ASCII word encoded: dropped. A first word that doesn't fit is written as-is, as on master, so the URL in a Subject: stays readable. A tab is now a normal fold point (CRLF + TAB, keeping the tab), not only near 998, so tab-separated text is no longer encoded.

Smaller stuff:

• PREV_ENDS_WITH_BACKSLASH is gone. It's now word::generatorState::lastCharIsBackslash, next to lastCharIsSpace. It's set from what was actually written, so a quoted or encoded word clears it. Note that this adds a member to a class in a public header.
• mailboxGroup::generateImpl() now moves whole mailboxes to the next line, like addressList, with a test in mailboxGroupTest.
• the leading tab: fixed in a separate commit, "no space in addition to a leading tab of an encoded-word". the tab is now used as the separator instead of being encoded after an extra space, so "foo@\tä" round-trips. When a fold is needed there, it becomes CRLF + TAB.
• parameterTest 81 chars: agreed, leaving that for a separate PR.

[vincent-richard]

Thanks, that was quick! I went through the new version:

• it builds and passes the parser tests, with a UTF-8 locale and with LC_ALL=C, and CI is green, CygWin included;

• I re-run the fuzzer: still no round-trip regressions against master, and the tab fix took care of most of what was left;

• all the cases from my review now behave as you describe (local parts and SMTP envelope, the "C:" example within 76, the URL in Subject:, tab folding, groups, "foo@\tä").

In the meantime, I discovered a specific case that still doesn't round-trip: a fold right before a tab. Your output is correct (CRLF + TAB), but VMime's own parser reads a fold plus the white-space after it as a single space ("abc\r\n\tdef" gives "abc def"). That's an old parser bug, not yours, I'll fix it separately.

Two small thing in the tests, if you feel like it:

• testFoldWhitespaceRuns case 3: the 68-char URL fits on the line at column 9 (9 + 68 = 77 <= 78), so it also passed with the previous version and doesn't covers the change. The URL has to be 70 to 74 chars long: long enough not to fit after column 9, but under 75, where a run is still encoded (maxRunLength >= maxLineLength - 3). Eg. with 72 chars, which the previous version encoded, plus a check so that the test keeps its point if the URL is ever changed:

// A first word which does not fit on the line is not encoded
const std::string url =
    "https://www.example.com/a/rather/long/path/to/the/page.html?id=123456789";

VASSERT("does not fit", 9 + url.length() > 78 && url.length() < 75);
VASSERT_EQ("3", url, vmime::text(url, vmime::charset("us-ascii")).generate(78, 9));

• testFoldPoints: the new length check only looks at the last line, and doesn't count "col". Checking every line, with the column offset, would be stronger.

Don't worry about the new generatorState member: we're still at 0.9, so the ABI can change as long as 1.0 isn't out (any day now, as it has been for years :-) ).

Thanks again for all the work on this!

[grammmike]

sounds good - easy to do -> I've taken both test suggestions and squashed them in.

• testFoldWhitespaceRuns case 3 now uses the 72-char URL, with the does not fit check.
• testFoldPoints now checks every line, counting col on the first one. the only exception is a first line without a fold point (e.g. Copy at column 77), which can't be folded earlier.

Don't worry about the new generatorState member: we're still at 0.9, so the ABI can change as long as 1.0 isn't out (any day now, as it has been for years :-) ).

generatorState ABI hint: thanks. On our side, nothing uses it directly, so no biggie.

Thanks again for all the work on this!

Thank you for taking care of the CRLF + TAB unfolding in the parser separately

In other note (probably not the right place, forgive me):

Big thanks to all of this here from us (grommunio) -> vmime is undoubtedly the best c++ mime parser there is. however, we think that it could be maintained maybe a bit more closely/frequently (also taking care of issues, etc.) . I have consulted our CTO and he is willing to offer substantial resources to this project if you are interested. not only code (like here), but also helping with issues and even more if it helps.

We love vmime and we like to help.

[vincent-richard]

All good, thanks!

[vincent-richard]

Thanks a lot for the kind words, and for the offer! It's great to hear that VMime is useful to you. :-)

To be honest, I have a day job (and a life outside of it!) that keeps me pretty busy. I've managed to free up some time these last few weeks to come back to VMime, a project I really care about, but I can't promise I'll always be this available. I'll do what I can.

In any case, contributions like this one are very welcome, and they make my life a lot easier.

[vincent-richard]

One last request before I merge: I'd like to use "Rebase and merge", so that your commits stay separate in the history rather than being squashed into one. Since GitHub doesn't let me edit the messages that way, could you reword the commit subjects to match the style of the history? Replace the "vmime:" prefix with "RFC 5322 Folding:" (or "Parser:" for the list-splitting one), and add "(#365)" at the end, eg. "RFC 5322 Folding: keep encoded-words within 75 chars (#365)". No need to touch the code. Thanks!

[grammmike]

done. hope this works for you.