Re: [PATCH proxmox] fix #7814: send-mail: normalize LF line endings to CRLF endings

["Shannon Sterz" <s.sterz@proxmox.com>]

On Wed Jul 29, 2026 at 10:18 AM CEST, Lukas Wagner wrote:
> Hi Shannon,
>
> sorry for the delay in getting to review this. Some notes inline.
>
> For what it's worth, this seems to work as advertised. Code also looks
> okay to me in its current form. If you decide to act upon my feedback,
> I'll quickly go over it again and send another R-b for that as well.
>
> Tested-by: Lukas Wagner <l.wagner@proxmox.com>
> Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>

thank you and i'll send a new version in a minute.

> On Tue Jul 14, 2026 at 10:47 AM CEST, Shannon Sterz wrote:
>> RFC5321 Section 2.3.8 requires that mails do not contain bare CR or LF
>> bytes. this patch normalizes unix style LF line endings to CRLF line
>> endings. it also removes bare CR bytes.
>>
>> when text would be base64 encoded anyway, CR bytes will be dropped, to
>> keep the behavior for encoded and non-encoded text aligned. however,
>> since this code was already extensively tested against multiple MUAs,
>> encoded LF line endings are kept as-is.
>>
>> also adds tests to encode this behavior.
>>
>> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
>> ---
>>  proxmox-sendmail/src/lib.rs | 246 ++++++++++++++++++++++++++++++++----
>>  1 file changed, 220 insertions(+), 26 deletions(-)
>>
>> diff --git a/proxmox-sendmail/src/lib.rs b/proxmox-sendmail/src/lib.rs
>> index b3ff56e5..a73e8eb7 100644
>> --- a/proxmox-sendmail/src/lib.rs
>> +++ b/proxmox-sendmail/src/lib.rs

-->8 snip 8<--

>> @@ -63,6 +66,18 @@ fn encode_filename_formatted(filename: &str) -> String {
>>      format_encoded_text(&encoded, format_prefix, ";", 0, true)
>>  }
>>
>> +/// Strip bare CR bytes, than base64 encode. This is useful to make sure we treat encoded (header)
>
> nit: should be 'then' instead of 'than'

done.

-->8 snip 8<--

>> @@ -485,11 +499,27 @@ impl<'a> Mail<'a> {
>>              .spawn()
>>              .with_context(|| "could not spawn sendmail process")?;
>>
>> +        let capacity = message.iter().fold(0, |acc, e| match *e {
>> +            b'\r' => acc,
>> +            b'\n' => acc + 2,
>> +            _ => acc + 1,
>> +        });
>
> No hard feelings, this is not a hot code path after all, but I guess you
> could just multiply message.len() with some constant factor (e.g. 2) to
> get the initial capacity and avoid the double iteration. The message is
> likely not going to be larger than a couple hundred bytes or a few
> kilobytes, so the extra memory does not matter IMO.

ack, i did that now and added a comment on why we chose to do that.

-->8 snip 8<--

>> @@ -606,17 +638,15 @@ impl<'a> Mail<'a> {
>>          header.push_str("Auto-Submitted: auto-generated;\n");
>>
>>          for (key, value) in &self.headers {
>> -            // Strip CR/LF so an interpolated key or value cannot inject extra header lines.
>> -            let key = key.replace(['\r', '\n'], "");
>> -            let value = value.replace(['\r', '\n'], "");
>> +            // Strip LF so an interpolated key or value cannot inject extra header lines. Bare CR
>> +            // will be stripped for non-ASCII text before encoding and for ASCII text globally
>> +            // before finishing formatting.
>> +            let key = key.replace('\n', "");
>> +            let value = value.replace('\n', "");
>
> I wonder whether it would be better to just return errors if there are
> control characters in Subject, headers, recipient... what do you think?

i'd prefer that too. however, this was added previously [1] and there
are now multiple internal users of this crate. so imo such a change is
a) orthogonal to this patch and b) warrants consultation with the
aforementioned internal users who might rely on this behavior.

[1]: https://git.proxmox.com/?p=proxmox.git;a=blobdiff;f=proxmox-sendmail/src/lib.rs;h=b3ff56e5ab