proxy/handler: rework args handling a little bit

jkarneges · jkarneges · commit ff1dd249d29a · 2025-10-07T19:56:14.000-07:00
diff --git a/src/core/config.rs b/src/core/config.rs
@@ -426,6 +426,10 @@ impl CustomConfig {
     }
 }
 
+pub fn default_config_file() -> PathBuf {
+    PathBuf::from(env!("CONFIG_DIR")).join("pushpin.conf")
+}
+
 pub fn get_config_file(
     work_dir: &Path,
     arg_config: Option<PathBuf>,
@@ -446,10 +450,7 @@ pub fn get_config_file(
                     .join("pushpin.conf"),
             );
             // default
-            config_files.push(PathBuf::from(format!(
-                "{}/pushpin.conf",
-                env!("CONFIG_DIR")
-            )));
+            config_files.push(default_config_file());
         }
     }
 
diff --git a/src/handler/cliargs.rs b/src/handler/cliargs.rs
@@ -14,10 +14,9 @@
  * limitations under the License.
  */
 
-use crate::core::config::get_config_file;
+use crate::core::config::default_config_file;
 use crate::core::version;
 use clap::{arg, Parser};
-use std::env;
 use std::ffi::CString;
 use std::path::PathBuf;
 
@@ -57,17 +56,18 @@ pub struct CliArgs {
 impl CliArgs {
     /// Verifies the command line arguments.
     pub fn verify(mut self) -> Self {
-        let work_dir = env::current_dir().unwrap_or_default();
-        let config_path: Option<PathBuf> = self.config_file.as_ref().map(PathBuf::from);
-
-        // Resolve the config file path using get_config_file
-        self.config_file = match get_config_file(&work_dir, config_path) {
-            Ok(path) => Some(path.to_string_lossy().to_string()),
-            Err(e) => {
-                eprintln!("error: failed to find configuration file: {}", e);
-                std::process::exit(1);
-            }
-        };
+        let config_file = self
+            .config_file
+            .map(PathBuf::from)
+            .unwrap_or_else(default_config_file);
+
+        // FIXME: don't put back as a string after resolving
+        self.config_file = Some(
+            config_file
+                .to_str()
+                .expect("path sourced from string should convert")
+                .to_string(),
+        );
 
         if self.verbose {
             self.log_level = 3;
@@ -81,12 +81,7 @@ impl CliArgs {
             .config_file
             .as_ref()
             .map_or_else(
-                || {
-                    let work_dir = std::env::current_dir().unwrap_or_default();
-                    let default_config = get_config_file(&work_dir, None)
-                        .unwrap_or_else(|_| "examples/config/pushpin.conf".into());
-                    CString::new(default_config.to_string_lossy().to_string()).unwrap()
-                },
+                || CString::new("").unwrap(),
                 |s| CString::new(s.as_str()).unwrap(),
             )
             .into_raw();
@@ -120,13 +115,15 @@ impl CliArgs {
 }
 
 pub mod ffi {
+    use std::ffi::{c_char, c_int, c_uint};
+
     #[repr(C)]
     pub struct HandlerCliArgs {
-        pub config_file: *mut libc::c_char,
-        pub log_file: *mut libc::c_char,
-        pub log_level: libc::c_uint,
-        pub ipc_prefix: *mut libc::c_char,
-        pub port_offset: libc::c_int,
+        pub config_file: *mut c_char,
+        pub log_file: *mut c_char,
+        pub log_level: c_uint,
+        pub ipc_prefix: *mut c_char,
+        pub port_offset: c_int,
     }
 }
 
@@ -174,8 +171,6 @@ mod tests {
             port_offset: Some(8080),
         };
 
-        let args_ffi = args.to_ffi();
-
         // Test verify() method
         let verified_args = args.verify();
         assert_eq!(verified_args.config_file, Some(config_test_file.clone()));
@@ -185,6 +180,8 @@ mod tests {
         assert_eq!(verified_args.ipc_prefix, Some("ipc".to_string()));
         assert_eq!(verified_args.port_offset, Some(8080));
 
+        let args_ffi = verified_args.to_ffi();
+
         // Test conversion to C++-compatible struct
         unsafe {
             assert_eq!(
@@ -224,14 +221,9 @@ mod tests {
             port_offset: None,
         };
 
-        let empty_args_ffi = empty_args.to_ffi();
-
         // Test verify() with empty args
         let verified_empty_args = empty_args.verify();
-        let default_config_file = get_config_file(&env::current_dir().unwrap(), None)
-            .unwrap()
-            .to_string_lossy()
-            .to_string();
+        let default_config_file = default_config_file().to_str().unwrap().to_string();
         assert_eq!(
             verified_empty_args.config_file,
             Some(default_config_file.clone())
@@ -242,6 +234,8 @@ mod tests {
         assert_eq!(verified_empty_args.ipc_prefix, None);
         assert_eq!(verified_empty_args.port_offset, None);
 
+        let empty_args_ffi = verified_empty_args.to_ffi();
+
         // Test conversion to C++-compatible struct
         unsafe {
             assert_eq!(
diff --git a/src/handler/handlerapp.cpp b/src/handler/handlerapp.cpp
@@ -104,9 +104,6 @@ class HandlerApp::Private
 		QCoreApplication::setApplicationVersion(Config::get().version);
 
 		HandlerArgsData args(argsFfi);
-		Settings settings(args.configFile);
-		if (!args.ipcPrefix.isEmpty()) settings.setIpcPrefix(args.ipcPrefix);
-		if (args.portOffset != -1) settings.setPortOffset(args.portOffset);
 
 		// Set the log level
 		if(args.logLevel != -1)
@@ -131,11 +128,15 @@ class HandlerApp::Private
 			QFile file(args.configFile);
 			if(!file.open(QIODevice::ReadOnly))
 			{
-				log_error("failed to open %s", qPrintable(args.configFile));
+				log_error("failed to open config file: %s", qPrintable(args.configFile));
 				return 1;
 			}
 		}
 
+		Settings settings(args.configFile);
+		if (!args.ipcPrefix.isEmpty()) settings.setIpcPrefix(args.ipcPrefix);
+		if (args.portOffset != -1) settings.setPortOffset(args.portOffset);
+
 		QStringList services = settings.value("runner/services").toStringList();
 
 		QStringList connmgr_in_stream_specs = settings.value("proxy/connmgr_in_stream_specs").toStringList();
diff --git a/src/proxy/cliargs.rs b/src/proxy/cliargs.rs
@@ -14,12 +14,12 @@
  * limitations under the License.
  */
 
-use crate::core::config::get_config_file;
+use crate::core::config::default_config_file;
 use crate::core::version;
 use clap::{arg, Parser};
-use std::env;
-use std::ffi::CString;
+use std::ffi::{c_char, CString};
 use std::path::PathBuf;
+use std::slice;
 
 // Struct to hold the command line arguments
 #[derive(Parser, Debug)]
@@ -57,17 +57,18 @@ pub struct CliArgs {
 impl CliArgs {
     /// Verifies the command line arguments.
     pub fn verify(mut self) -> Self {
-        let work_dir = env::current_dir().unwrap_or_default();
-        let config_path: Option<PathBuf> = self.config_file.as_ref().map(PathBuf::from);
-
-        // Resolve the config file path using get_config_file
-        self.config_file = match get_config_file(&work_dir, config_path) {
-            Ok(path) => Some(path.to_string_lossy().to_string()),
-            Err(e) => {
-                eprintln!("error: failed to find configuration file: {}", e);
-                std::process::exit(1);
-            }
-        };
+        let config_file = self
+            .config_file
+            .map(PathBuf::from)
+            .unwrap_or_else(default_config_file);
+
+        // FIXME: don't put back as a string after resolving
+        self.config_file = Some(
+            config_file
+                .to_str()
+                .expect("path sourced from string should convert")
+                .to_string(),
+        );
 
         if self.verbose {
             self.log_level = 3;
@@ -81,12 +82,7 @@ impl CliArgs {
             .config_file
             .as_ref()
             .map_or_else(
-                || {
-                    let work_dir = std::env::current_dir().unwrap_or_default();
-                    let default_config = get_config_file(&work_dir, None)
-                        .unwrap_or_else(|_| "examples/config/pushpin.conf".into());
-                    CString::new(default_config.to_string_lossy().to_string()).unwrap()
-                },
+                || CString::new("").unwrap(),
                 |s| CString::new(s.as_str()).unwrap(),
             )
             .into_raw();
@@ -109,47 +105,47 @@ impl CliArgs {
             )
             .into_raw();
 
-        let (routes, routes_count) = if !self.route.is_empty() {
-            // Allocate array of string pointers
-            let routes_array = unsafe {
-                libc::malloc(self.route.len() * std::mem::size_of::<*mut libc::c_char>())
-                    as *mut *mut libc::c_char
-            };
-
-            // Convert each route to CString and store pointer in array
-            for (i, item) in self.route.iter().enumerate() {
-                let c_string = CString::new(item.to_string()).unwrap().into_raw();
-                unsafe {
-                    *routes_array.add(i) = c_string;
-                }
-            }
-
-            (routes_array, self.route.len() as libc::c_uint)
-        } else {
-            let routes_array = unsafe { libc::malloc(0) as *mut *mut libc::c_char };
-            (routes_array, 0)
-        };
+        let mut routes: Vec<*mut c_char> = Vec::new();
+
+        // Convert each route to CString and store pointer in array
+        for item in &self.route {
+            let item = item.as_bytes();
+
+            // ensure no trailing nul byte
+            let end = item.iter().position(|b| *b == 0).unwrap_or(item.len());
+            let item = &item[..end];
+
+            let c_string = CString::new(item).unwrap().into_raw();
+            routes.push(c_string);
+        }
+
+        let routes = routes.into_boxed_slice();
+
+        let routes_count = routes.len();
+        let routes: *mut [*mut c_char] = Box::into_raw(routes);
 
         ffi::ProxyCliArgs {
             config_file,
             log_file,
             log_level: self.log_level,
             ipc_prefix,
-            routes,
+            routes: routes as *mut *mut c_char,
             routes_count,
         }
     }
 }
 
 pub mod ffi {
+    use std::ffi::{c_char, c_uint};
+
     #[repr(C)]
     pub struct ProxyCliArgs {
-        pub config_file: *mut libc::c_char,
-        pub log_file: *mut libc::c_char,
-        pub log_level: libc::c_uint,
-        pub ipc_prefix: *mut libc::c_char,
-        pub routes: *mut *mut libc::c_char,
-        pub routes_count: libc::c_uint,
+        pub config_file: *mut c_char,
+        pub log_file: *mut c_char,
+        pub log_level: c_uint,
+        pub ipc_prefix: *mut c_char,
+        pub routes: *mut *mut c_char,
+        pub routes_count: libc::size_t,
     }
 }
 
@@ -177,15 +173,18 @@ pub unsafe extern "C" fn destroy_proxy_cli_args(ffi_args: ffi::ProxyCliArgs) {
         let _ = CString::from_raw(ffi_args.ipc_prefix);
     }
     if !ffi_args.routes.is_null() {
+        // SAFETY: the raw parts were originally derived from a boxed slice
+        let routes = unsafe {
+            Box::from_raw(slice::from_raw_parts_mut(
+                ffi_args.routes,
+                ffi_args.routes_count,
+            ))
+        };
+
         // Free each individual route string
-        for i in 0..ffi_args.routes_count {
-            let route_ptr = *ffi_args.routes.add(i as usize);
-            if !route_ptr.is_null() {
-                let _ = CString::from_raw(route_ptr);
-            }
+        for item in routes.iter() {
+            let _ = CString::from_raw(*item);
         }
-
-        libc::free(ffi_args.routes as *mut libc::c_void);
     }
 }
 
@@ -209,8 +208,6 @@ mod tests {
             route: vec!["route1".to_string(), "route2".to_string()],
         };
 
-        let args_ffi = args.to_ffi();
-
         // Test verify() method
         let verified_args = args.verify();
         assert_eq!(verified_args.config_file, Some(config_test_file.clone()));
@@ -223,6 +220,8 @@ mod tests {
             vec!["route1".to_string(), "route2".to_string()]
         );
 
+        let args_ffi = verified_args.to_ffi();
+
         // Test conversion to C++-compatible struct
         unsafe {
             assert_eq!(
@@ -277,14 +276,9 @@ mod tests {
             route: Vec::new(),
         };
 
-        let empty_args_ffi = empty_args.to_ffi();
-
         // Test verify() with empty args
         let verified_empty_args = empty_args.verify();
-        let default_config_file = get_config_file(&env::current_dir().unwrap(), None)
-            .unwrap()
-            .to_string_lossy()
-            .to_string();
+        let default_config_file = default_config_file().to_str().unwrap().to_string();
         assert_eq!(
             verified_empty_args.config_file,
             Some(default_config_file.clone())
@@ -295,6 +289,8 @@ mod tests {
         assert_eq!(verified_empty_args.ipc_prefix, None);
         assert!(verified_empty_args.route.is_empty());
 
+        let empty_args_ffi = verified_empty_args.to_ffi();
+
         // Test conversion to C++-compatible struct
         unsafe {
             assert_eq!(
diff --git a/src/proxy/proxyapp.cpp b/src/proxy/proxyapp.cpp

Original file line number	Diff line number	Diff line change
`@@ -426,6 +426,10 @@ impl CustomConfig {`
`426`	`426`	`}`
`427`	`427`	`}`
`428`	`428`
	`429`	`+pub fn default_config_file() -> PathBuf {`
	`430`	`+ PathBuf::from(env!("CONFIG_DIR")).join("pushpin.conf")`
	`431`	`+}`
	`432`	`+`
`429`	`433`	`pub fn get_config_file(`
`430`	`434`	`work_dir: &Path,`
`431`	`435`	`arg_config: Option<PathBuf>,`
`@@ -446,10 +450,7 @@ pub fn get_config_file(`
`446`	`450`	`.join("pushpin.conf"),`
`447`	`451`	`);`
`448`	`452`	`// default`
`449`		`- config_files.push(PathBuf::from(format!(`
`450`		`- "{}/pushpin.conf",`
`451`		`- env!("CONFIG_DIR")`
`452`		`- )));`
	`453`	`+ config_files.push(default_config_file());`
`453`	`454`	`}`
`454`	`455`	`}`
`455`	`456`