Fix permission denied on workload socket (Azure#5705)

Co-authored-by: Ubuntu <azureuser@debug.wciqoipclkaejnvszvrhsyebkf.bx.internal.cloudapp.net>

edgelet/edgelet-http/Cargo.toml

@@ -34,7 +34,6 @@ systemd = { path = "../systemd" }
 hyperlocal = "0.6"
 libc = "0.2"
 nix = "0.14"
-scopeguard = "0.3.3"
 tokio-uds = "0.2"
 
 [target.'cfg(windows)'.dependencies]

edgelet/edgelet-http/src/unix.rs

@@ -2,18 +2,12 @@
 
 use std::fs;
 #[cfg(unix)]
-use std::os::unix::fs::MetadataExt;
-#[cfg(unix)]
 use std::os::unix::prelude::PermissionsExt;
 use std::path::Path;
 
 use failure::ResultExt;
 use log::{debug, error};
 #[cfg(unix)]
-use nix::sys::stat::{umask, Mode};
-#[cfg(unix)]
-use scopeguard::defer;
-#[cfg(unix)]
 use tokio_uds::UnixListener;
 #[cfg(windows)]
 use tokio_uds_windows::UnixListener;
@@ -22,56 +16,36 @@ use crate::error::{Error, ErrorKind};
 use crate::util::{incoming::Incoming, socket_file_exists};
 
 pub fn listener<P: AsRef<Path>>(path: P, unix_socket_permission: u32) -> Result<Incoming, Error> {
-    let listener = if socket_file_exists(path.as_ref()) {
-        // get the previous file's metadata
-        #[cfg(unix)]
-        let metadata = get_metadata(path.as_ref())?;
-
-        debug!("unlinking {}...", path.as_ref().display());
-        fs::remove_file(&path)
-            .with_context(|_| ErrorKind::Path(path.as_ref().display().to_string()))?;
-        debug!("unlinked {}", path.as_ref().display());
-
-        #[cfg(unix)]
-        let prev = set_umask(&metadata, path.as_ref());
-        #[cfg(unix)]
-        defer! {{ umask(prev); }}
-
-        debug!("binding {}...", path.as_ref().display());
-
-        let listener = UnixListener::bind(&path)
-            .with_context(|_| ErrorKind::Path(path.as_ref().display().to_string()))?;
-        debug!("bound {}", path.as_ref().display());
-
-        Incoming::Unix(listener)
-    } else {
-        // If parent doesn't exist, create it and socket will be created inside.
-        if let Some(parent) = path.as_ref().parent() {
-            if !parent.exists() {
-                fs::create_dir_all(parent).with_context(|err| {
-                    error!("Cannot create directory, error: {}", err);
-                    ErrorKind::Path(path.as_ref().display().to_string())
-                })?;
-            }
-        }
+    let path = path.as_ref();
+    let path_display = path.display();
 
-        let listener = UnixListener::bind(&path)
-            .with_context(|_| ErrorKind::Path(path.as_ref().display().to_string()))?;
+    if socket_file_exists(path) {
+        debug!("unlinking {}...", path_display);
 
-        set_permissions(path.as_ref(), unix_socket_permission)?;
+        let err1 = fs::remove_file(path).err();
+        let err2 = fs::remove_dir_all(path).err();
+        if let Some((err1, err2)) = err1.zip(err2) {
+            error!("Could not unlink existing socket: [{}] [{}]", err1, err2);
+            return Err(ErrorKind::Path(path_display.to_string()).into());
+        }
+        debug!("unlinked {}", path_display);
+    }
 
-        Incoming::Unix(listener)
-    };
+    // If parent doesn't exist, create it and socket will be created inside.
+    if let Some(parent) = path.parent() {
+        fs::create_dir_all(parent).with_context(|err| {
+            error!("Cannot create directory, error: {}", err);
+            ErrorKind::Path(path_display.to_string())
+        })?;
+    }
 
-    Ok(listener)
-}
+    let listener =
+        UnixListener::bind(&path).with_context(|_| ErrorKind::Path(path_display.to_string()))?;
+    debug!("bound {}", path_display);
 
-#[cfg(unix)]
-fn get_metadata(path: &Path) -> Result<fs::Metadata, Error> {
-    let metadata =
-        fs::metadata(path).with_context(|_| ErrorKind::Path(path.display().to_string()))?;
-    debug!("read metadata {:?} for {}", metadata, path.display());
-    Ok(metadata)
+    set_permissions(path, unix_socket_permission)?;
+
+    Ok(Incoming::Unix(listener))
 }
 
 #[cfg(unix)]
@@ -91,29 +65,12 @@ fn set_permissions(_path: &Path, _unix_socket_permission: u32) -> Result<(), Err
     Ok(())
 }
 
-#[cfg(unix)]
-fn set_umask(metadata: &fs::Metadata, path: &Path) -> Mode {
-    #[cfg(target_os = "macos")]
-    #[allow(clippy::cast_possible_truncation)]
-    let mode = Mode::from_bits_truncate(metadata.mode() as u16);
-
-    #[cfg(not(target_os = "macos"))]
-    let mode = Mode::from_bits_truncate(metadata.mode());
-
-    let mut mask = Mode::all();
-    mask.toggle(mode);
-
-    debug!("settings permissions {:#o} for {}...", mode, path.display());
-
-    umask(mask)
-}
-
 #[cfg(test)]
 #[cfg(unix)]
 mod tests {
-    use super::{listener, MetadataExt};
-
+    use super::listener;
     use std::fs::OpenOptions;
+    use std::os::unix::fs::MetadataExt;
     use std::os::unix::fs::OpenOptionsExt;
 
     use futures::Stream;
@@ -139,7 +96,21 @@ mod tests {
         let _srv = listener.for_each(move |(_socket, _addr)| Ok(()));
 
         let file_stat = stat(&path).unwrap();
-        assert_eq!(0o600, file_stat.st_mode & 0o777);
+        assert_eq!(0o666, file_stat.st_mode & 0o777);
+
+        dir.close().unwrap();
+    }
+
+    #[test]
+    fn test_socket_not_created() {
+        let dir = tempdir().unwrap();
+        let path = dir.path().join("dummy_socket.sock");
+
+        let listener = listener(&path, 0o666).unwrap();
+        let _srv = listener.for_each(move |(_socket, _addr)| Ok(()));
+
+        let file_stat = stat(&path).unwrap();
+        assert_eq!(0o666, file_stat.st_mode & 0o777);
 
         dir.close().unwrap();
     }

edgelet/iotedged/src/workload_manager.rs

@@ -133,14 +133,33 @@ where
     ) -> Result<(), Error> {
         let label = "work".to_string();
 
-        let (shutdown_sender, shutdown_receiver) = oneshot::channel();
+        // If a listener has already been created, remove previous listener.
+        // This avoid the launch of 2 listeners.
+        // We chose to remove instead and create a new one instead of
+        // just return and say, one listener has already been created:
+        // We chose to remove because a listener could crash without getting removed correctly.
+        // That could make the module crash. Then that module would be restarted without ever going to
+        // "stop"
+        // There is still a chance that 2 concurrent servers are launch with concurrence,
+        // But it is extremely unlikely and anyway doesn't have any side effect expect memory footprint.
+        if let Some(shutdown_sender) = self.shutdown_senders.remove(module_id) {
+            info!(
+                "Listener  {} already started, removing old listener",
+                module_id
+            );
+            shutdown_sender
+                .send(())
+                .map_err(|()| Error::from(ErrorKind::WorkloadService))?;
+        }
 
-        let cert_manager = self.cert_manager.clone();
-        let min_protocol_version = self.min_protocol_version;
+        let (shutdown_sender, shutdown_receiver) = oneshot::channel();
 
         self.shutdown_senders
             .insert(module_id.to_string(), shutdown_sender);
 
+        let cert_manager = self.cert_manager.clone();
+        let min_protocol_version = self.min_protocol_version;
+
         let future = WorkloadService::new(
             &self.key_store,
             self.crypto.clone(),