[pbs-devel] [PATCH backup] mark setup_safe_path_env as unsafe

[pbs-devel] [PATCH backup] mark setup_safe_path_env as unsafe

[Maximiliano Sandoval]

This method is unsafe in the 2024 edition.

The specific wording used in the safety docstring comes from `cargo fix
--edition`.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---

I would personally prefer if someone more familiarized with these 4 binaries can
clarify whether all these uses are safe and add a, for example,
```
SAFETY: The enviroment is only ever accessed from here.
```
comment on top of the unsafe incantations.


 src/bin/proxmox-backup-api.rs     |  2 +-
 src/bin/proxmox-backup-manager.rs |  2 +-
 src/bin/proxmox-backup-proxy.rs   |  2 +-
 src/bin/proxmox-daily-update.rs   |  2 +-
 src/tools/mod.rs                  | 16 ++++++++++++----
 5 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/src/bin/proxmox-backup-api.rs b/src/bin/proxmox-backup-api.rs
index 829974d25..ed1d12772 100644
--- a/src/bin/proxmox-backup-api.rs
+++ b/src/bin/proxmox-backup-api.rs
@@ -20,7 +20,7 @@ use proxmox_backup::server::auth::check_pbs_auth;
 fn main() {
     pbs_tools::setup_libc_malloc_opts();
 
-    proxmox_backup::tools::setup_safe_path_env();
+    unsafe { proxmox_backup::tools::setup_path_env() };
 
     if let Err(err) = proxmox_async::runtime::main(run()) {
         eprintln!("Error: {}", err);
diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs
index 02ca0d028..910e9623d 100644
--- a/src/bin/proxmox-backup-manager.rs
+++ b/src/bin/proxmox-backup-manager.rs
@@ -704,7 +704,7 @@ async fn run() -> Result<(), Error> {
 }
 
 fn main() -> Result<(), Error> {
-    proxmox_backup::tools::setup_safe_path_env();
+    unsafe { proxmox_backup::tools::setup_path_env() };
 
     proxmox_async::runtime::main(run())
 }
diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index 2b6ceb6e6..a2bc3d3f1 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -57,7 +57,7 @@ use proxmox_backup::server::do_verification_job;
 fn main() -> Result<(), Error> {
     pbs_tools::setup_libc_malloc_opts();
 
-    proxmox_backup::tools::setup_safe_path_env();
+    unsafe { proxmox_backup::tools::setup_path_env() };
 
     let backup_uid = pbs_config::backup_user()?.uid;
     let backup_gid = pbs_config::backup_group()?.gid;
diff --git a/src/bin/proxmox-daily-update.rs b/src/bin/proxmox-daily-update.rs
index dbfee07b4..7861c9a79 100644
--- a/src/bin/proxmox-daily-update.rs
+++ b/src/bin/proxmox-daily-update.rs
@@ -108,7 +108,7 @@ async fn run(rpcenv: &mut dyn RpcEnvironment) -> Result<(), Error> {
 }
 
 fn main() {
-    proxmox_backup::tools::setup_safe_path_env();
+    unsafe { proxmox_backup::tools::setup_path_env() };
 
     if let Err(err) = syslog::init(
         syslog::Facility::LOG_DAEMON,
diff --git a/src/tools/mod.rs b/src/tools/mod.rs
index 322894dd7..220ec445e 100644
--- a/src/tools/mod.rs
+++ b/src/tools/mod.rs
@@ -54,10 +54,18 @@ pub fn pbs_simple_http(proxy_config: Option<ProxyConfig>) -> Client {
     Client::with_options(options)
 }
 
-pub fn setup_safe_path_env() {
-    std::env::set_var("PATH", "/sbin:/bin:/usr/sbin:/usr/bin");
-    // Make %ENV safer - as suggested by https://perldoc.perl.org/perlsec.html
+/// Setups the enviroment in a safer way.
+///
+/// Here "safer" should be understood as described in
+/// https://perldoc.perl.org/perlsec.html.
+///
+/// ## Safety
+///
+/// The caller must ensure that enviroment access only happens in
+/// single-threaded code.
+pub unsafe fn setup_path_env() {
+    unsafe { std::env::set_var("PATH", "/sbin:/bin:/usr/sbin:/usr/bin") };
     for name in &["IFS", "CDPATH", "ENV", "BASH_ENV"] {
-        std::env::remove_var(name);
+        unsafe { std::env::remove_var(name) };
     }
 }
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel