[pve-devel] [PATCH proxmox v2 0/5] sys: email: always format 'Date' header with C locale

[pve-devel] [PATCH proxmox v2 0/5] sys: email: always format 'Date' header with C locale

[Lukas Wagner]

This patch series makes the formatting of the 'Date' header for sent emails
independent of the system locale. Previously, strftime was used to generate
the header, but since this function is locale-aware, invalid Date headers
could be generated.
As an example, if the locale was `de_DE.UTF-8`, then 

Date: Di, 05 Dez 2023 12:05:50 +0100
 instead of  
Date: Tue, 05 Dec 2023 12:05:50 +0100

would be generated, violating RFC2822 [1]. This trips up some email clients 
(e.g. KMail), making them fall back to Jan 1970 (epoch 0).

This is fixed by adding `strftime_l`, which allows passing a locale to
used. Additionally, `epoch_to_rfc2822` was added, which formats a 
unix epoch value to a valid RFC2822 date string (with a fixed "C" locale).

Popped up in our forum:
https://forum.proxmox.com/threads/pve-mail-notification-kein-datum-mehr-zum-korrekten-einsortieren.137556

The following projects need to be bumped and rebuilt:
  - proxmox_sys, and therefore
      - proxmox_notify, and therefore
          - libpve-rs-perl 
          - proxmox-mail-forward
      - proxmox-backup (uses proxmox_sys::email::sendmail directly)

[1] https://www.rfc-editor.org/rfc/rfc2822#section-3.3

Changes v1 -> v2:
 - Fix TZ-dependent test case

Lukas Wagner (5):
  time: posix: use strftime from the `libc` crate.
  time: posix: inline vars in string formatting
  time: posix: add bindings for strftime_l
  time: posix: add epoch_to_rfc2822
  sys: email: use `epoch_to_rfc2822` from proxmox_time

 proxmox-sys/src/email.rs  |   3 +-
 proxmox-time/src/posix.rs | 151 +++++++++++++++++++++++++++++++-------
 2 files changed, 126 insertions(+), 28 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH proxmox v2 1/5] time: posix: use strftime from the `libc` crate.

[Lukas Wagner]

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-time/src/posix.rs | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/proxmox-time/src/posix.rs b/proxmox-time/src/posix.rs
index 6157f8b..c463ab3 100644
--- a/proxmox-time/src/posix.rs
+++ b/proxmox-time/src/posix.rs
@@ -108,25 +108,13 @@ pub fn epoch_f64() -> f64 {
     }
 }
 
-//  rust libc bindings do not include strftime
-#[link(name = "c")]
-extern "C" {
-    #[link_name = "strftime"]
-    fn libc_strftime(
-        s: *mut libc::c_char,
-        max: libc::size_t,
-        format: *const libc::c_char,
-        time: *const libc::tm,
-    ) -> libc::size_t;
-}
-
 /// Safe bindings to libc strftime
 pub fn strftime(format: &str, t: &libc::tm) -> Result<String, Error> {
     let format = CString::new(format).map_err(|err| format_err!("{}", err))?;
     let mut buf = vec![0u8; 8192];
 
     let res = unsafe {
-        libc_strftime(
+        libc::strftime(
             buf.as_mut_ptr() as *mut libc::c_char,
             buf.len() as libc::size_t,
             format.as_ptr(),
-- 
2.39.2

[pve-devel] [PATCH proxmox v2 2/5] time: posix: inline vars in string formatting

[Lukas Wagner]

No functional changes.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-time/src/posix.rs | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/proxmox-time/src/posix.rs b/proxmox-time/src/posix.rs
index c463ab3..5913816 100644
--- a/proxmox-time/src/posix.rs
+++ b/proxmox-time/src/posix.rs
@@ -13,7 +13,7 @@ pub fn timelocal(t: &mut libc::tm) -> Result<i64, Error> {
 
     let epoch = unsafe { libc::mktime(t) };
     if epoch == -1 {
-        bail!("libc::mktime failed for {:?}", t);
+        bail!("libc::mktime failed for {t:?}");
     }
     Ok(epoch)
 }
@@ -27,7 +27,7 @@ pub fn timegm(t: &mut libc::tm) -> Result<i64, Error> {
 
     let epoch = unsafe { libc::timegm(t) };
     if epoch == -1 {
-        bail!("libc::timegm failed for {:?}", t);
+        bail!("libc::timegm failed for {t:?}");
     }
     Ok(epoch)
 }
@@ -54,7 +54,7 @@ pub fn localtime(epoch: i64) -> Result<libc::tm, Error> {
 
     unsafe {
         if libc::localtime_r(&epoch, &mut result).is_null() {
-            bail!("libc::localtime failed for '{}'", epoch);
+            bail!("libc::localtime failed for '{epoch}'");
         }
     }
 
@@ -67,7 +67,7 @@ pub fn gmtime(epoch: i64) -> Result<libc::tm, Error> {
 
     unsafe {
         if libc::gmtime_r(&epoch, &mut result).is_null() {
-            bail!("libc::gmtime failed for '{}'", epoch);
+            bail!("libc::gmtime failed for '{epoch}'");
         }
     }
 
@@ -110,7 +110,7 @@ pub fn epoch_f64() -> f64 {
 
 /// Safe bindings to libc strftime
 pub fn strftime(format: &str, t: &libc::tm) -> Result<String, Error> {
-    let format = CString::new(format).map_err(|err| format_err!("{}", err))?;
+    let format = CString::new(format).map_err(|err| format_err!("{err}"))?;
     let mut buf = vec![0u8; 8192];
 
     let res = unsafe {
@@ -135,7 +135,7 @@ pub fn strftime(format: &str, t: &libc::tm) -> Result<String, Error> {
         bail!("strftime: result len is 0 (string too large)");
     };
 
-    let c_str = CStr::from_bytes_with_nul(&buf[..len + 1]).map_err(|err| format_err!("{}", err))?;
+    let c_str = CStr::from_bytes_with_nul(&buf[..len + 1]).map_err(|err| format_err!("{err}"))?;
     let str_slice: &str = c_str.to_str().unwrap();
     Ok(str_slice.to_owned())
 }
@@ -158,7 +158,7 @@ pub fn epoch_to_rfc3339_utc(epoch: i64) -> Result<String, Error> {
 
     let year = gmtime.tm_year + 1900;
     if year < 0 || year > 9999 {
-        bail!("epoch_to_rfc3339_utc: wrong year '{}'", year);
+        bail!("epoch_to_rfc3339_utc: wrong year '{year}'");
     }
 
     strftime("%010FT%TZ", &gmtime)
@@ -172,7 +172,7 @@ pub fn epoch_to_rfc3339(epoch: i64) -> Result<String, Error> {
 
     let year = localtime.tm_year + 1900;
     if year < 0 || year > 9999 {
-        bail!("epoch_to_rfc3339: wrong year '{}'", year);
+        bail!("epoch_to_rfc3339: wrong year '{year}'");
     }
 
     // Note: We cannot use strftime %z because of missing collon
@@ -192,20 +192,15 @@ pub fn epoch_to_rfc3339(epoch: i64) -> Result<String, Error> {
 
     let mut s = strftime("%10FT%T", &localtime)?;
     s.push(prefix);
-    let _ = write!(s, "{:02}:{:02}", hours, mins);
+    let _ = write!(s, "{hours:02}:{mins:02}");
 
     Ok(s)
 }
 
 /// Parse RFC3339 into Unix epoch
 pub fn parse_rfc3339(input_str: &str) -> Result<i64, Error> {
-    parse_rfc3339_do(input_str).map_err(|err| {
-        format_err!(
-            "failed to parse rfc3339 timestamp ({:?}) - {}",
-            input_str,
-            err
-        )
-    })
+    parse_rfc3339_do(input_str)
+        .map_err(|err| format_err!("failed to parse rfc3339 timestamp ({input_str:?}) - {err}",))
 }
 
 fn parse_rfc3339_do(input_str: &str) -> Result<i64, Error> {
@@ -213,7 +208,7 @@ fn parse_rfc3339_do(input_str: &str) -> Result<i64, Error> {
 
     let expect = |pos: usize, c: u8| {
         if input[pos] != c {
-            bail!("unexpected char at pos {}", pos);
+            bail!("unexpected char at pos {pos}");
         }
         Ok(())
     };
@@ -221,14 +216,14 @@ fn parse_rfc3339_do(input_str: &str) -> Result<i64, Error> {
     let digit = |pos: usize| -> Result<i32, Error> {
         let digit = input[pos] as i32;
         if digit < 48 || digit > 57 {
-            bail!("unexpected char at pos {}", pos);
+            bail!("unexpected char at pos {pos}");
         }
         Ok(digit - 48)
     };
 
     fn check_max(i: i32, max: i32) -> Result<i32, Error> {
         if i > max {
-            bail!("value too large ({} > {})", i, max);
+            bail!("value too large ({i} > {max})");
         }
         Ok(i)
     }
-- 
2.39.2

[pve-devel] [PATCH proxmox v2 3/5] time: posix: add bindings for strftime_l

[Lukas Wagner]

This variant of strftime can be provided with a locale_t, which
determines the locale used for time formatting.

A struct `Locale` was also introduced as a safe wrapper around
locale_t.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-time/src/posix.rs | 99 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/proxmox-time/src/posix.rs b/proxmox-time/src/posix.rs
index 5913816..9c8002a 100644
--- a/proxmox-time/src/posix.rs
+++ b/proxmox-time/src/posix.rs
@@ -140,6 +140,93 @@ pub fn strftime(format: &str, t: &libc::tm) -> Result<String, Error> {
     Ok(str_slice.to_owned())
 }
 
+//  The `libc` crate does not yet contain bindings for `strftime_l`
+#[link(name = "c")]
+extern "C" {
+    #[link_name = "strftime_l"]
+    fn libc_strftime_l(
+        s: *mut libc::c_char,
+        max: libc::size_t,
+        format: *const libc::c_char,
+        time: *const libc::tm,
+        locale: libc::locale_t,
+    ) -> libc::size_t;
+}
+
+/// Safe bindings to libc's `strftime_l`
+///
+/// The compared to `strftime`, `strftime_l` allows the caller to provide a
+/// locale object.
+pub fn strftime_l(format: &str, t: &libc::tm, locale: &Locale) -> Result<String, Error> {
+    let format = CString::new(format).map_err(|err| format_err!("{err}"))?;
+    let mut buf = vec![0u8; 8192];
+
+    let res = unsafe {
+        libc_strftime_l(
+            buf.as_mut_ptr() as *mut libc::c_char,
+            buf.len() as libc::size_t,
+            format.as_ptr(),
+            t as *const libc::tm,
+            locale.locale,
+        )
+    };
+    if res == !0 {
+        // -1,, it's unsigned
+        // man strftime: POSIX.1-2001  does  not  specify  any  errno  settings   for strftime()
+        bail!("strftime failed");
+    }
+
+    // `res` is a `libc::size_t`, which on a different target architecture might not be directly
+    // assignable to a `usize`. Thus, we actually want a cast here.
+    #[allow(clippy::unnecessary_cast)]
+    let len = res as usize;
+
+    if len == 0 {
+        bail!("strftime: result len is 0 (string too large)");
+    };
+
+    let c_str = CStr::from_bytes_with_nul(&buf[..len + 1]).map_err(|err| format_err!("{err}"))?;
+    let str_slice: &str = c_str.to_str()?;
+    Ok(str_slice.to_owned())
+}
+
+/// A safe wrapper for `libc::locale_t`.
+///
+/// The enclosed locale object will be automatically freed by `Drop::drop`
+pub struct Locale {
+    locale: libc::locale_t,
+}
+
+impl Locale {
+    /// Portable, minimal locale environment
+    const C: &'static str = "C";
+
+    /// Create a new locale object.
+    /// `category_mask` is expected to be a bitmask based on the
+    /// LC_*_MASK constants from libc. The locale will be set to `locale`
+    /// for every entry in the bitmask which is set.
+    pub fn new(category_mask: i32, locale: &str) -> Result<Self, Error> {
+        let format = CString::new(locale).map_err(|err| format_err!("{err}"))?;
+
+        let locale =
+            unsafe { libc::newlocale(category_mask, format.as_ptr(), std::ptr::null_mut()) };
+
+        if locale.is_null() {
+            return Err(std::io::Error::last_os_error().into());
+        }
+
+        Ok(Self { locale })
+    }
+}
+
+impl Drop for Locale {
+    fn drop(&mut self) {
+        unsafe {
+            libc::freelocale(self.locale);
+        }
+    }
+}
+
 /// Format epoch as local time
 pub fn strftime_local(format: &str, epoch: i64) -> Result<String, Error> {
     let localtime = localtime(epoch)?;
@@ -391,3 +478,15 @@ fn test_timezones() {
     let res = epoch_to_rfc3339_utc(parsed).expect("converting to RFC failed");
     assert_eq!(expected_utc, res);
 }
+
+#[test]
+fn test_strftime_l() {
+    let epoch = 1609263000;
+
+    let locale = Locale::new(libc::LC_ALL, Locale::C).expect("could not create locale");
+    let time = gmtime(epoch).expect("gmtime failed");
+
+    let formatted = strftime_l("%a, %d %b %Y %T %z", &time, &locale).expect("strftime_l failed");
+
+    assert_eq!(formatted, "Tue, 29 Dec 2020 17:30:00 +0000");
+}
-- 
2.39.2

[pve-devel] [PATCH proxmox v2 4/5] time: posix: add epoch_to_rfc2822

[Lukas Wagner]

This is the format used in the 'Date' header in mails.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
Changes v1 -> v2:
 - Fix TZ-dependent test case

 proxmox-time/src/posix.rs | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/proxmox-time/src/posix.rs b/proxmox-time/src/posix.rs
index 9c8002a..73a5368 100644
--- a/proxmox-time/src/posix.rs
+++ b/proxmox-time/src/posix.rs
@@ -371,6 +371,15 @@ fn parse_rfc3339_do(input_str: &str) -> Result<i64, Error> {
     Ok(epoch)
 }
 
+/// Convert Unix epoch into RFC2822 local time with TZ
+pub fn epoch_to_rfc2822(epoch: i64) -> Result<String, Error> {
+    let localtime = localtime(epoch)?;
+    let locale = Locale::new(libc::LC_ALL, Locale::C)?;
+    let rfc2822_date = strftime_l("%a, %d %b %Y %T %z", &localtime, &locale)?;
+
+    Ok(rfc2822_date)
+}
+
 #[test]
 fn test_leap_seconds() {
     let convert_reconvert = |epoch| {
@@ -490,3 +499,11 @@ fn test_strftime_l() {
 
     assert_eq!(formatted, "Tue, 29 Dec 2020 17:30:00 +0000");
 }
+
+#[test]
+fn test_epoch_to_rfc2822() {
+    let epoch = 1609263000;
+    // Output is TZ-dependent, so only verify that it did not fail.
+    // Internally, it uses strftime_l which we test already.
+    assert!(epoch_to_rfc2822(epoch).is_ok());
+}
-- 
2.39.2

[pve-devel] [PATCH proxmox v2 5/5] sys: email: use `epoch_to_rfc2822` from proxmox_time

[Lukas Wagner]

`strftime`'s formatting is locale-dependent. If the system locale was
set to e.g. de_DE.UTF-8, the `Date` header became invalid
(e.g Mo instead of Mon for 'Monday'), tripping up some mail clients
(e.g. KMail).

This commit should fix this by using the new `epoch_to_rfc2822`
function from proxmox_time. Under the hood, this function uses
`strftime_l` with a fixed locale (C).

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-sys/src/email.rs | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/proxmox-sys/src/email.rs b/proxmox-sys/src/email.rs
index be92f30..85d171d 100644
--- a/proxmox-sys/src/email.rs
+++ b/proxmox-sys/src/email.rs
@@ -63,8 +63,7 @@ pub fn sendmail(
     }
     let _ = writeln!(body, "From: {} <{}>", author, mailfrom);
     let _ = writeln!(body, "To: {}", &recipients);
-    let localtime = proxmox_time::localtime(now)?;
-    let rfc2822_date = proxmox_time::strftime("%a, %d %b %Y %T %z", &localtime)?;
+    let rfc2822_date = proxmox_time::epoch_to_rfc2822(now)?;
     let _ = writeln!(body, "Date: {}", rfc2822_date);
     body.push_str("Auto-Submitted: auto-generated;\n");
 
-- 
2.39.2

Re: [pve-devel] [PATCH proxmox v2 0/5] sys: email: always format 'Date' header with C locale

[Lukas Wagner]

On 12/11/23 14:29, Lukas Wagner wrote:
> This patch series makes the formatting of the 'Date' header for sent emails
> independent of the system locale. Previously, strftime was used to generate
> the header, but since this function is locale-aware, invalid Date headers
> could be generated.
> As an example, if the locale was `de_DE.UTF-8`, then
> 
> Date: Di, 05 Dez 2023 12:05:50 +0100
>   instead of
> Date: Tue, 05 Dec 2023 12:05:50 +0100
> 
> would be generated, violating RFC2822 [1]. This trips up some email clients
> (e.g. KMail), making them fall back to Jan 1970 (epoch 0).
> 
> This is fixed by adding `strftime_l`, which allows passing a locale to
> used. Additionally, `epoch_to_rfc2822` was added, which formats a
> unix epoch value to a valid RFC2822 date string (with a fixed "C" locale).
> 
> Popped up in our forum:
> https://forum.proxmox.com/threads/pve-mail-notification-kein-datum-mehr-zum-korrekten-einsortieren.137556
> 
> The following projects need to be bumped and rebuilt:
>    - proxmox_sys, and therefore
>        - proxmox_notify, and therefore
>            - libpve-rs-perl
>            - proxmox-mail-forward
>        - proxmox-backup (uses proxmox_sys::email::sendmail directly)
> 
> [1] https://www.rfc-editor.org/rfc/rfc2822#section-3.3
> 
> Changes v1 -> v2:
>   - Fix TZ-dependent test case
> 
> Lukas Wagner (5):
>    time: posix: use strftime from the `libc` crate.
>    time: posix: inline vars in string formatting
>    time: posix: add bindings for strftime_l
>    time: posix: add epoch_to_rfc2822
>    sys: email: use `epoch_to_rfc2822` from proxmox_time
> 
>   proxmox-sys/src/email.rs  |   3 +-
>   proxmox-time/src/posix.rs | 151 +++++++++++++++++++++++++++++++-------
>   2 files changed, 126 insertions(+), 28 deletions(-)
> 

ping

-- 
- Lukas

[pve-devel] applied-series: [PATCH proxmox v2 0/5] sys: email: always format 'Date' header with C locale

[Wolfgang Bumiller]

applied series, thanks

not 100% sure about the `C` const - might be nice to instead have a
static Lazy<Locale> C to get via `Locale::c()` but that can happen
later...