From 1823f6a178a4413f9e1c0c20e6a5765b7e079a9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20B=C3=BCrgin?= Date: Wed, 18 Jan 2023 10:12:45 +0100 Subject: [PATCH] Bump MSRV to 1.61.0, replace panicking [e]println! --- CHANGELOG.md | 13 ++++++++ Cargo.toml | 2 +- README.md | 4 +-- src/callbacks.rs | 12 +++---- src/client.rs | 22 ++++++------- src/email.rs | 14 ++++----- src/lib.rs | 34 +++++++++++++++++--- src/main.rs | 55 ++++++++++++++++----------------- tests/ham_message.rs | 2 +- tests/reject_spam.rs | 2 +- tests/skip_oversized.rs | 2 +- tests/spam_message.rs | 2 +- tests/spamc_connection_error.rs | 2 +- 13 files changed, 100 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23df3fb..0346fec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,18 @@ # SpamAssassin Milter changelog +## 0.4.0 (unreleased) + +The minimum supported Rust version is now 1.61.0. + +### Changed + +* The minimum supported Rust version has been raised to 1.61.0. + +### Fixed + +* The program no longer panics when it cannot write to standard error during + operation. + ## 0.3.2 (2022-08-31) ### Changed diff --git a/Cargo.toml b/Cargo.toml index 5c55189..87d6617 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,7 +2,7 @@ name = "spamassassin-milter" version = "0.3.2" edition = "2021" -rust-version = "1.56.1" +rust-version = "1.61.0" description = "Milter for spam filtering with SpamAssassin" license = "GPL-3.0-or-later" categories = ["email"] diff --git a/README.md b/README.md index 4abedf7..b779ca7 100644 --- a/README.md +++ b/README.md @@ -50,7 +50,7 @@ server. By default, `/usr/bin/spamc` is used as the executable. To override this, set the environment variable `SPAMASSASSIN_MILTER_SPAMC` to the desired path when building the application. -The minimum supported Rust version is 1.56.1. +The minimum supported Rust version is 1.61.0. [Rust]: https://www.rust-lang.org @@ -261,7 +261,7 @@ messages with score 5.0 or above into ‘Junk’. ## Licence -Copyright © 2020–2022 David Bürgin +Copyright © 2020–2023 David Bürgin This program is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software diff --git a/src/callbacks.rs b/src/callbacks.rs index 5911bbf..5930308 100644 --- a/src/callbacks.rs +++ b/src/callbacks.rs @@ -38,7 +38,7 @@ use std::{ macro_rules! ok_or_tempfail { ($expr:expr) => { if let ::std::result::Result::Err(e) = $expr { - ::std::eprintln!("failed to communicate with spamc: {}", e); + eprintln!("failed to communicate with spamc: {e}"); return ::indymilter::Status::Tempfail; } }; @@ -149,7 +149,7 @@ async fn handle_connect( if config.use_trusted_networks() { if config.is_in_trusted_networks(&ip) { - verbose!(config, "accepted connection from trusted network address {}", ip); + verbose!(config, "accepted connection from trusted network address {ip}"); return Status::Accept; } } else if ip.is_loopback() { @@ -179,7 +179,7 @@ async fn handle_mail( ) -> Status { if !config.auth_untrusted() { if let Some(login) = context.macros.get_string(c_str!("{auth_authen}")) { - verbose!(config, "accepted message from sender authenticated as \"{}\"", login); + verbose!(config, "accepted message from sender authenticated as \"{login}\""); return Status::Accept; } } @@ -212,7 +212,7 @@ async fn handle_data(context: &mut Context) -> Status { let id = context.macros.queue_id(); if let Err(e) = client.connect() { - eprintln!("{}: failed to start spamc: {}", id, e); + eprintln!("{id}: failed to start spamc: {e}"); return Status::Tempfail; } @@ -274,7 +274,7 @@ async fn handle_body( let max = config.max_message_size(); if client.bytes_written() > max { let id = context.macros.queue_id(); - verbose!(config, "{}: skipping rest of message larger than {} bytes", id, max); + verbose!(config, "{id}: skipping rest of message larger than {max} bytes"); client.skip_body(); Status::Skip } else { @@ -291,7 +291,7 @@ async fn handle_eom(config: Arc, context: &mut EomContext) - match client.process(&id, &mut context.reply, &context.actions, &config).await { Ok(status) => status, Err(e) => { - eprintln!("{}: failed to process message: {}", id, e); + eprintln!("{id}: failed to process message: {e}"); Status::Tempfail } } diff --git a/src/client.rs b/src/client.rs index b25b7a7..525dae9 100644 --- a/src/client.rs +++ b/src/client.rs @@ -114,7 +114,7 @@ impl Process for Spamc { "spamc terminated by signal {}", status.signal().unwrap() )), - Some(code) => Error::Io(format!("spamc exited with status code {}", code)), + Some(code) => Error::Io(format!("spamc exited with status code {code}")), }) } } @@ -236,7 +236,7 @@ impl Client { // As requested during milter protocol negotiation, the value includes // leading whitespace. This lets us pass on whitespace exactly as is. let value = email::ensure_crlf(value); - let buf = format!("{}:{}\r\n", name, value); + let buf = format!("{name}:{value}\r\n"); if email::is_spam_assassin_header(name) || email::REWRITE_HEADERS.contains(name) @@ -277,7 +277,7 @@ impl Client { let output = match self.process.finish().await { Ok(output) => output, Err(e) => { - eprintln!("{}: failed to complete spamc communication: {}", id, e); + eprintln!("{id}: failed to complete spamc communication: {e}"); return Ok(Status::Tempfail); } }; @@ -285,7 +285,7 @@ impl Client { let email = match Email::parse(&output) { Ok(email) => email, Err(e) => { - eprintln!("{}: invalid response from spamc: {}", id, e); + eprintln!("{id}: invalid response from spamc: {e}"); return Ok(Status::Tempfail); } }; @@ -319,7 +319,7 @@ impl Client { fn reject_spam(id: &str, reply: &mut impl SetErrorReply, config: &Config) -> Result { if config.dry_run() { - verbose!(config, "{}: rejected message flagged as spam [dry run, not done]", id); + verbose!(config, "{id}: rejected message flagged as spam [dry run, not done]"); Ok(Status::Accept) } else { reply.set_error_reply( @@ -328,7 +328,7 @@ fn reject_spam(id: &str, reply: &mut impl SetErrorReply, config: &Config) -> Res config.reply_text().lines(), )?; - verbose!(config, "{}: rejected message flagged as spam", id); + verbose!(config, "{id}: rejected message flagged as spam"); Ok(if config.reply_code().starts_with('5') { Status::Reject } else { @@ -349,9 +349,8 @@ async fn replace_body( // This condition ensures message integrity in such a misconfigured setup. if skipped { eprintln!( - "{}: not replacing possibly truncated message body; \ + "{id}: not replacing possibly truncated message body; \ please review max message size setting ({})", - id, config.max_message_size() ); } else { @@ -589,10 +588,9 @@ mod tests { assert_eq!( as_mock_spamc(client.process.as_ref()).buf, format!( - "X-Envelope-From: {}\r\n\ - X-Envelope-To: {},\r\n\ - \t{}\r\n", - sender, recipient1, recipient2 + "X-Envelope-From: {sender}\r\n\ + X-Envelope-To: {recipient1},\r\n\ + \t{recipient2}\r\n", ) .as_bytes() ); diff --git a/src/email.rs b/src/email.rs index 419c421..03f7c47 100644 --- a/src/email.rs +++ b/src/email.rs @@ -300,9 +300,9 @@ where { for m in mods { if config.dry_run() { - verbose!(config, "{}: rewriting header: {} [dry run, not done]", id, m); + verbose!(config, "{id}: rewriting header: {m} [dry run, not done]"); } else { - verbose!(config, "{}: rewriting header: {}", id, m); + verbose!(config, "{id}: rewriting header: {m}"); m.execute(actions).await?; } } @@ -316,9 +316,9 @@ pub async fn replace_body( config: &Config, ) -> Result<()> { if config.dry_run() { - verbose!(config, "{}: replacing message body [dry run, not done]", id); + verbose!(config, "{id}: replacing message body [dry run, not done]"); } else { - verbose!(config, "{}: replacing message body", id); + verbose!(config, "{id}: replacing message body"); actions.replace_body(body).await?; } Ok(()) @@ -377,11 +377,11 @@ impl Display for HeaderMod<'_> { use HeaderMod::*; match self { - Add { name, .. } => write!(f, "add header \"{}\"", name), + Add { name, .. } => write!(f, "add header \"{name}\""), Replace { name, .. } | Modify { name, .. } => { - write!(f, "replace header \"{}\"", name) + write!(f, "replace header \"{name}\"") } - Delete { name } => write!(f, "delete header \"{}\"", name), + Delete { name } => write!(f, "delete header \"{name}\""), } } } diff --git a/src/lib.rs b/src/lib.rs index 7714f84..5a1abc8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -23,10 +23,23 @@ //! //! [SpamAssassin Milter]: https://crates.io/crates/spamassassin-milter +// The standard `eprintln` macro is replaced throughout with a best-effort, +// non-panicking version. +macro_rules! eprintln { + ($($arg:tt)*) => { + { + use ::std::io::Write; + let _ = ::std::writeln!(::std::io::stderr(), $($arg)*); + } + }; +} + macro_rules! verbose { ($config:ident, $($arg:tt)*) => { if $config.verbose() { - ::std::eprintln!($($arg)*); + // Note: not qualifying `eprintln!` here makes it use textual scope + // and thus refer to above definition. + eprintln!($($arg)*); } }; } @@ -40,7 +53,10 @@ mod error; pub use crate::config::{Config, ConfigBuilder}; use indymilter::IntoListener; -use std::{future::Future, io}; +use std::{ + future::Future, + io::{self, stderr, Write}, +}; /// The name of the SpamAssassin Milter application. pub const MILTER_NAME: &str = "SpamAssassin Milter"; @@ -67,7 +83,7 @@ pub const VERSION: &str = env!("CARGO_PKG_VERSION"); /// let shutdown = signal::ctrl_c(); /// /// if let Err(e) = spamassassin_milter::run(listener, config, shutdown).await { -/// eprintln!("failed to run spamassassin-milter: {}", e); +/// eprintln!("failed to run spamassassin-milter: {e}"); /// process::exit(1); /// } /// # Ok(()) @@ -81,5 +97,15 @@ pub async fn run( let callbacks = callbacks::make_callbacks(config); let config = Default::default(); - indymilter::run(listener, callbacks, config, shutdown).await + // Propagate any I/O error here: don’t start if stderr cannot be written to. + writeln!(stderr(), "{MILTER_NAME} {VERSION} starting")?; + + let result = indymilter::run(listener, callbacks, config, shutdown).await; + + match &result { + Ok(()) => eprintln!("{MILTER_NAME} {VERSION} shut down"), + Err(e) => eprintln!("{MILTER_NAME} {VERSION} terminated with error: {e}"), + } + + result } diff --git a/src/main.rs b/src/main.rs index 6d426ae..2999047 100644 --- a/src/main.rs +++ b/src/main.rs @@ -20,7 +20,14 @@ use signal_hook::consts::{SIGINT, SIGTERM}; use signal_hook_tokio::{Handle, Signals}; use spamassassin_milter::{Config, MILTER_NAME, VERSION}; use std::{ - env, error::Error, net::IpAddr, os::unix::fs::FileTypeExt, path::Path, process, str::FromStr, + env, + error::Error, + io::{stderr, stdout, Write}, + net::IpAddr, + os::unix::fs::FileTypeExt, + path::Path, + process, + str::FromStr, }; use tokio::{ fs, @@ -29,12 +36,14 @@ use tokio::{ task::JoinHandle, }; +const PROGRAM_NAME: &str = env!("CARGO_BIN_NAME"); + #[tokio::main] async fn main() { let (socket, config) = match parse_args() { Ok(config) => config, Err(e) => { - eprintln!("error: {}", e); + let _ = writeln!(stderr(), "{PROGRAM_NAME}: {e}"); process::exit(1); } }; @@ -52,7 +61,7 @@ async fn main() { let listener = match TcpListener::bind(socket).await { Ok(listener) => listener, Err(e) => { - eprintln!("error: could not bind TCP socket: {}", e); + let _ = writeln!(stderr(), "{PROGRAM_NAME}: could not bind TCP socket: {e}"); process::exit(1); } }; @@ -68,7 +77,7 @@ async fn main() { let listener = match UnixListener::bind(socket) { Ok(listener) => listener, Err(e) => { - eprintln!("error: could not create UNIX domain socket: {}", e); + let _ = writeln!(stderr(), "{PROGRAM_NAME}: could not create UNIX domain socket: {e}"); process::exit(1); } }; @@ -81,20 +90,12 @@ async fn main() { } }; - eprintln!("{} {} starting", MILTER_NAME, VERSION); - let result = spamassassin_milter::run(listener, config, shutdown).await; cleanup(signals_handle, signals_task, socket_path).await; - match result { - Ok(()) => { - eprintln!("{} {} shut down", MILTER_NAME, VERSION); - } - Err(e) => { - eprintln!("{} {} terminated with error: {}", MILTER_NAME, VERSION, e); - process::exit(1); - } + if result.is_err() { + process::exit(1); } } @@ -112,14 +113,13 @@ impl FromStr for Socket { } else if let Some(s) = s.strip_prefix("unix:") { Ok(Self::Unix(s.into())) } else { - Err(format!("invalid value for socket: \"{}\"", s)) + Err(format!("invalid value for socket: \"{s}\"")) } } } const USAGE_TEXT: &str = "\ -Usage: - spamassassin-milter [OPTIONS] [--] [...] +[OPTIONS] [--] [...] Arguments: Listening socket of the milter @@ -154,17 +154,15 @@ fn parse_args() -> Result<(Socket, Config), Box> { let socket = loop { let arg = args.next().ok_or("required argument missing")??; - let missing_value = || format!("missing value for option {}", arg); + let missing_value = || format!("missing value for option {arg}"); match arg.as_str() { "-h" | "--help" => { - println!("{} {}", MILTER_NAME, VERSION); - println!(); - print!("{}", USAGE_TEXT); + write!(stdout(), "Usage: {PROGRAM_NAME} {USAGE_TEXT}")?; process::exit(0); } "-V" | "--version" => { - println!("{} {}", MILTER_NAME, VERSION); + writeln!(stdout(), "{MILTER_NAME} {VERSION}")?; process::exit(0); } "-a" | "--auth-untrusted" => { @@ -188,7 +186,7 @@ fn parse_args() -> Result<(Socket, Config), Box> { "-s" | "--max-message-size" => { let arg = args.next().ok_or_else(missing_value)??; let bytes = arg.parse() - .map_err(|_| format!("invalid value for max message size: \"{}\"", arg))?; + .map_err(|_| format!("invalid value for max message size: \"{arg}\""))?; config = config.max_message_size(bytes); } @@ -217,7 +215,7 @@ fn parse_args() -> Result<(Socket, Config), Box> { // supported. let net = net.parse() .or_else(|_| net.parse::().map(From::from)) - .map_err(|_| format!("invalid value for trusted network address: \"{}\"", net))?; + .map_err(|_| format!("invalid value for trusted network address: \"{net}\""))?; config = config.trusted_network(net); } @@ -229,7 +227,7 @@ fn parse_args() -> Result<(Socket, Config), Box> { if arg.starts_with('-') && arg.chars().all(|c| c.is_ascii_alphanumeric() || c == '-') { - return Err(format!("unrecognized option: \"{}\"", arg).into()); + return Err(format!("unrecognized option: \"{arg}\"").into()); } break arg.parse()?; @@ -270,16 +268,15 @@ fn validate_reply_codes( if !((c1.starts_with('4') || c1.starts_with('5')) && c2.starts_with(&c1[..1])) => { Err(format!( - "invalid or incompatible values for reply code and status code: \"{}\", \"{}\"", - c1, c2 + "invalid or incompatible values for reply code and status code: \"{c1}\", \"{c2}\"" ) .into()) } (Some(c), None) if !c.starts_with('5') => { - Err(format!("invalid value for reply code (5XX): \"{}\"", c).into()) + Err(format!("invalid value for reply code (5XX): \"{c}\"").into()) } (None, Some(c)) if !c.starts_with('5') => { - Err(format!("invalid value for reply status code (5.X.X): \"{}\"", c).into()) + Err(format!("invalid value for reply status code (5.X.X): \"{c}\"").into()) } _ => Ok(()), } diff --git a/tests/ham_message.rs b/tests/ham_message.rs index 0940479..17e1ded 100644 --- a/tests/ham_message.rs +++ b/tests/ham_message.rs @@ -6,7 +6,7 @@ use spamassassin_milter::*; #[tokio::test] async fn ham_message() { let config = configure_spamc(Config::builder()) - .spamc_args(&[format!("--port={}", SPAMD_PORT)]) + .spamc_args([format!("--port={SPAMD_PORT}")]) .build(); let server = spawn_mock_spamd_server(SPAMD_PORT, |ham| { diff --git a/tests/reject_spam.rs b/tests/reject_spam.rs index 5a7fb96..36e1d1e 100644 --- a/tests/reject_spam.rs +++ b/tests/reject_spam.rs @@ -9,7 +9,7 @@ async fn reject_spam() { .reject_spam(true) .reply_code("554".into()) .reply_text("Not allowed!".into()) - .spamc_args(&[format!("--port={}", SPAMD_PORT)]) + .spamc_args([format!("--port={SPAMD_PORT}")]) .build(); let server = spawn_mock_spamd_server(SPAMD_PORT, |spam| { diff --git a/tests/skip_oversized.rs b/tests/skip_oversized.rs index 4801ee8..7b6ba32 100644 --- a/tests/skip_oversized.rs +++ b/tests/skip_oversized.rs @@ -7,7 +7,7 @@ use spamassassin_milter::*; async fn skip_oversized() { let config = configure_spamc(Config::builder()) .max_message_size(512) - .spamc_args(&[format!("--port={}", SPAMD_PORT)]) + .spamc_args([format!("--port={SPAMD_PORT}")]) .build(); let server = spawn_mock_spamd_server(SPAMD_PORT, Ok).await.unwrap(); diff --git a/tests/spam_message.rs b/tests/spam_message.rs index b58d2d3..d147328 100644 --- a/tests/spam_message.rs +++ b/tests/spam_message.rs @@ -6,7 +6,7 @@ use spamassassin_milter::*; #[tokio::test] async fn spam_message() { let config = configure_spamc(Config::builder()) - .spamc_args(&[format!("--port={}", SPAMD_PORT)]) + .spamc_args([format!("--port={SPAMD_PORT}")]) .build(); let server = spawn_mock_spamd_server(SPAMD_PORT, |spam| { diff --git a/tests/spamc_connection_error.rs b/tests/spamc_connection_error.rs index 75be941..0de2406 100644 --- a/tests/spamc_connection_error.rs +++ b/tests/spamc_connection_error.rs @@ -6,7 +6,7 @@ use spamassassin_milter::*; #[tokio::test] async fn spamc_connection_error() { let config = configure_spamc(Config::builder()) - .spamc_args(&[format!("--port={}", SPAMD_PORT)]) + .spamc_args([format!("--port={SPAMD_PORT}")]) .build(); let milter = SpamAssassinMilter::spawn(LOCALHOST, config).await.unwrap();