net: refactor named pipe builders to not use bitfields (#5477)

Author: mhils

tokio/src/net/windows/named_pipe.rs

@@ -1645,28 +1645,24 @@ impl AsRawHandle for NamedPipeClient {
     }
 }
 
-// Helper to set a boolean flag as a bitfield.
-macro_rules! bool_flag {
-    ($f:expr, $t:expr, $flag:expr) => {{
-        let current = $f;
-
-        if $t {
-            $f = current | $flag;
-        } else {
-            $f = current & !$flag;
-        };
-    }};
-}
-
 /// A builder structure for construct a named pipe with named pipe-specific
 /// options. This is required to use for named pipe servers who wants to modify
 /// pipe-related options.
 ///
 /// See [`ServerOptions::create`].
 #[derive(Debug, Clone)]
 pub struct ServerOptions {
-    open_mode: u32,
-    pipe_mode: u32,
+    // dwOpenMode
+    access_inbound: bool,
+    access_outbound: bool,
+    first_pipe_instance: bool,
+    write_dac: bool,
+    write_owner: bool,
+    access_system_security: bool,
+    // dwPipeMode
+    pipe_mode: PipeMode,
+    reject_remote_clients: bool,
+    // other options
     max_instances: u32,
     out_buffer_size: u32,
     in_buffer_size: u32,
@@ -1687,8 +1683,14 @@ impl ServerOptions {
     /// ```
     pub fn new() -> ServerOptions {
         ServerOptions {
-            open_mode: windows_sys::PIPE_ACCESS_DUPLEX | windows_sys::FILE_FLAG_OVERLAPPED,
-            pipe_mode: windows_sys::PIPE_TYPE_BYTE | windows_sys::PIPE_REJECT_REMOTE_CLIENTS,
+            access_inbound: true,
+            access_outbound: true,
+            first_pipe_instance: false,
+            write_dac: false,
+            write_owner: false,
+            access_system_security: false,
+            pipe_mode: PipeMode::Byte,
+            reject_remote_clients: true,
             max_instances: windows_sys::PIPE_UNLIMITED_INSTANCES,
             out_buffer_size: 65536,
             in_buffer_size: 65536,
@@ -1705,14 +1707,7 @@ impl ServerOptions {
     ///
     /// [`dwPipeMode`]: https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createnamedpipea
     pub fn pipe_mode(&mut self, pipe_mode: PipeMode) -> &mut Self {
-        let is_msg = matches!(pipe_mode, PipeMode::Message);
-        // Pipe mode is implemented as a bit flag 0x4. Set is message and unset
-        // is byte.
-        bool_flag!(
-            self.pipe_mode,
-            is_msg,
-            windows_sys::PIPE_TYPE_MESSAGE | windows_sys::PIPE_READMODE_MESSAGE
-        );
+        self.pipe_mode = pipe_mode;
         self
     }
 
@@ -1808,7 +1803,7 @@ impl ServerOptions {
     /// # Ok(()) }
     /// ```
     pub fn access_inbound(&mut self, allowed: bool) -> &mut Self {
-        bool_flag!(self.open_mode, allowed, windows_sys::PIPE_ACCESS_INBOUND);
+        self.access_inbound = allowed;
         self
     }
 
@@ -1906,7 +1901,7 @@ impl ServerOptions {
     /// # Ok(()) }
     /// ```
     pub fn access_outbound(&mut self, allowed: bool) -> &mut Self {
-        bool_flag!(self.open_mode, allowed, windows_sys::PIPE_ACCESS_OUTBOUND);
+        self.access_outbound = allowed;
         self
     }
 
@@ -1974,11 +1969,7 @@ impl ServerOptions {
     /// [`create`]: ServerOptions::create
     /// [`FILE_FLAG_FIRST_PIPE_INSTANCE`]: https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createnamedpipea#pipe_first_pipe_instance
     pub fn first_pipe_instance(&mut self, first: bool) -> &mut Self {
-        bool_flag!(
-            self.open_mode,
-            first,
-            windows_sys::FILE_FLAG_FIRST_PIPE_INSTANCE
-        );
+        self.first_pipe_instance = first;
         self
     }
 
@@ -2060,7 +2051,7 @@ impl ServerOptions {
     ///
     /// [`WRITE_DAC`]: https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createnamedpipea
     pub fn write_dac(&mut self, requested: bool) -> &mut Self {
-        bool_flag!(self.open_mode, requested, windows_sys::WRITE_DAC);
+        self.write_dac = requested;
         self
     }
 
@@ -2070,7 +2061,7 @@ impl ServerOptions {
     ///
     /// [`WRITE_OWNER`]: https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createnamedpipea
     pub fn write_owner(&mut self, requested: bool) -> &mut Self {
-        bool_flag!(self.open_mode, requested, windows_sys::WRITE_OWNER);
+        self.write_owner = requested;
         self
     }
 
@@ -2080,11 +2071,7 @@ impl ServerOptions {
     ///
     /// [`ACCESS_SYSTEM_SECURITY`]: https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createnamedpipea
     pub fn access_system_security(&mut self, requested: bool) -> &mut Self {
-        bool_flag!(
-            self.open_mode,
-            requested,
-            windows_sys::ACCESS_SYSTEM_SECURITY
-        );
+        self.access_system_security = requested;
         self
     }
 
@@ -2095,11 +2082,7 @@ impl ServerOptions {
     ///
     /// [`PIPE_REJECT_REMOTE_CLIENTS`]: https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createnamedpipea#pipe_reject_remote_clients
     pub fn reject_remote_clients(&mut self, reject: bool) -> &mut Self {
-        bool_flag!(
-            self.pipe_mode,
-            reject,
-            windows_sys::PIPE_REJECT_REMOTE_CLIENTS
-        );
+        self.reject_remote_clients = reject;
         self
     }
 
@@ -2245,10 +2228,46 @@ impl ServerOptions {
     ) -> io::Result<NamedPipeServer> {
         let addr = encode_addr(addr);
 
+        let pipe_mode = {
+            let mut mode = if matches!(self.pipe_mode, PipeMode::Message) {
+                windows_sys::PIPE_TYPE_MESSAGE | windows_sys::PIPE_READMODE_MESSAGE
+            } else {
+                windows_sys::PIPE_TYPE_BYTE | windows_sys::PIPE_READMODE_BYTE
+            };
+            if self.reject_remote_clients {
+                mode |= windows_sys::PIPE_REJECT_REMOTE_CLIENTS;
+            } else {
+                mode |= windows_sys::PIPE_ACCEPT_REMOTE_CLIENTS;
+            }
+            mode
+        };
+        let open_mode = {
+            let mut mode = windows_sys::FILE_FLAG_OVERLAPPED;
+            if self.access_inbound {
+                mode |= windows_sys::PIPE_ACCESS_INBOUND;
+            }
+            if self.access_outbound {
+                mode |= windows_sys::PIPE_ACCESS_OUTBOUND;
+            }
+            if self.first_pipe_instance {
+                mode |= windows_sys::FILE_FLAG_FIRST_PIPE_INSTANCE;
+            }
+            if self.write_dac {
+                mode |= windows_sys::WRITE_DAC;
+            }
+            if self.write_owner {
+                mode |= windows_sys::WRITE_OWNER;
+            }
+            if self.access_system_security {
+                mode |= windows_sys::ACCESS_SYSTEM_SECURITY;
+            }
+            mode
+        };
+
         let h = windows_sys::CreateNamedPipeW(
             addr.as_ptr(),
-            self.open_mode,
-            self.pipe_mode,
+            open_mode,
+            pipe_mode,
             self.max_instances,
             self.out_buffer_size,
             self.in_buffer_size,
@@ -2270,7 +2289,8 @@ impl ServerOptions {
 /// See [`ClientOptions::open`].
 #[derive(Debug, Clone)]
 pub struct ClientOptions {
-    desired_access: u32,
+    generic_read: bool,
+    generic_write: bool,
     security_qos_flags: u32,
     pipe_mode: PipeMode,
 }
@@ -2291,7 +2311,8 @@ impl ClientOptions {
     /// ```
     pub fn new() -> Self {
         Self {
-            desired_access: windows_sys::GENERIC_READ | windows_sys::GENERIC_WRITE,
+            generic_read: true,
+            generic_write: true,
             security_qos_flags: windows_sys::SECURITY_IDENTIFICATION
                 | windows_sys::SECURITY_SQOS_PRESENT,
             pipe_mode: PipeMode::Byte,
@@ -2305,7 +2326,7 @@ impl ClientOptions {
     /// [`GENERIC_READ`]: https://docs.microsoft.com/en-us/windows/win32/secauthz/generic-access-rights
     /// [`CreateFile`]: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew
     pub fn read(&mut self, allowed: bool) -> &mut Self {
-        bool_flag!(self.desired_access, allowed, windows_sys::GENERIC_READ);
+        self.generic_read = allowed;
         self
     }
 
@@ -2316,7 +2337,7 @@ impl ClientOptions {
     /// [`GENERIC_WRITE`]: https://docs.microsoft.com/en-us/windows/win32/secauthz/generic-access-rights
     /// [`CreateFile`]: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew
     pub fn write(&mut self, allowed: bool) -> &mut Self {
-        bool_flag!(self.desired_access, allowed, windows_sys::GENERIC_WRITE);
+        self.generic_write = allowed;
         self
     }
 
@@ -2434,13 +2455,24 @@ impl ClientOptions {
     ) -> io::Result<NamedPipeClient> {
         let addr = encode_addr(addr);
 
+        let desired_access = {
+            let mut access = 0;
+            if self.generic_read {
+                access |= windows_sys::GENERIC_READ;
+            }
+            if self.generic_write {
+                access |= windows_sys::GENERIC_WRITE;
+            }
+            access
+        };
+
         // NB: We could use a platform specialized `OpenOptions` here, but since
         // we have access to windows_sys it ultimately doesn't hurt to use
         // `CreateFile` explicitly since it allows the use of our already
         // well-structured wide `addr` to pass into CreateFileW.
         let h = windows_sys::CreateFileW(
             addr.as_ptr(),
-            self.desired_access,
+            desired_access,
             0,
             attrs as *mut _,
             windows_sys::OPEN_EXISTING,
@@ -2453,13 +2485,9 @@ impl ClientOptions {
         }
 
         if matches!(self.pipe_mode, PipeMode::Message) {
-            let mut mode = windows_sys::PIPE_READMODE_MESSAGE;
-            let result = windows_sys::SetNamedPipeHandleState(
-                h,
-                &mut mode,
-                ptr::null_mut(),
-                ptr::null_mut(),
-            );
+            let mode = windows_sys::PIPE_READMODE_MESSAGE;
+            let result =
+                windows_sys::SetNamedPipeHandleState(h, &mode, ptr::null_mut(), ptr::null_mut());
 
             if result == 0 {
                 return Err(io::Error::last_os_error());
@@ -2582,53 +2610,3 @@ unsafe fn named_pipe_info(handle: RawHandle) -> io::Result<PipeInfo> {
         max_instances,
     })
 }
-
-#[cfg(test)]
-mod test {
-    use self::windows_sys::{
-        PIPE_READMODE_MESSAGE, PIPE_REJECT_REMOTE_CLIENTS, PIPE_TYPE_BYTE, PIPE_TYPE_MESSAGE,
-    };
-    use super::*;
-
-    #[test]
-    fn opts_default_pipe_mode() {
-        let opts = ServerOptions::new();
-        assert_eq!(opts.pipe_mode, PIPE_TYPE_BYTE | PIPE_REJECT_REMOTE_CLIENTS);
-    }
-
-    #[test]
-    fn opts_unset_reject_remote() {
-        let mut opts = ServerOptions::new();
-        opts.reject_remote_clients(false);
-        assert_eq!(opts.pipe_mode & PIPE_REJECT_REMOTE_CLIENTS, 0);
-    }
-
-    #[test]
-    fn opts_set_pipe_mode_maintains_reject_remote_clients() {
-        let mut opts = ServerOptions::new();
-        opts.pipe_mode(PipeMode::Byte);
-        assert_eq!(opts.pipe_mode, PIPE_TYPE_BYTE | PIPE_REJECT_REMOTE_CLIENTS);
-
-        opts.reject_remote_clients(false);
-        opts.pipe_mode(PipeMode::Byte);
-        assert_eq!(opts.pipe_mode, PIPE_TYPE_BYTE);
-
-        opts.reject_remote_clients(true);
-        opts.pipe_mode(PipeMode::Byte);
-        assert_eq!(opts.pipe_mode, PIPE_TYPE_BYTE | PIPE_REJECT_REMOTE_CLIENTS);
-
-        opts.reject_remote_clients(false);
-        opts.pipe_mode(PipeMode::Message);
-        assert_eq!(opts.pipe_mode, PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE);
-
-        opts.reject_remote_clients(true);
-        opts.pipe_mode(PipeMode::Message);
-        assert_eq!(
-            opts.pipe_mode,
-            PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_REJECT_REMOTE_CLIENTS
-        );
-
-        opts.pipe_mode(PipeMode::Byte);
-        assert_eq!(opts.pipe_mode, PIPE_TYPE_BYTE | PIPE_REJECT_REMOTE_CLIENTS);
-    }
-}