windows: fixed #82

Author: elast0ny

Cargo.toml

@@ -35,7 +35,7 @@ nix = "0.23"
 libc = "0.2"
 
 [target.'cfg(windows)'.dependencies]
-win-sys = "0"
+win-sys = "0.2"
 
 [dev-dependencies]
 raw_sync = "0.1"

src/lib.rs

@@ -13,7 +13,7 @@ use cfg_if::cfg_if;
 cfg_if! {
     if #[cfg(feature = "logging")] {
         pub use log;
-    } else {    
+    } else {
         #[allow(unused_macros)]
         mod log {
             macro_rules! trace (($($tt:tt)*) => {{}});
@@ -259,7 +259,7 @@ impl Shmem {
     }
     /// Returns a raw pointer to the mapping
     pub fn as_ptr(&self) -> *mut u8 {
-        self.mapping.map_ptr
+        self.mapping.as_mut_ptr()
     }
     /// Returns mapping as a byte slice
     /// # Safety

src/windows.rs

@@ -2,49 +2,39 @@ use std::fs::{File, OpenOptions};
 use std::io::ErrorKind;
 use std::os::windows::{fs::OpenOptionsExt, io::AsRawHandle};
 use std::path::PathBuf;
-use std::ptr::null_mut;
 
 use crate::log::*;
 use win_sys::*;
-use win_sys::Foundation::INVALID_HANDLE_VALUE;
 
 use crate::ShmemError;
 
 pub struct MapData {
     owner: bool,
 
-    ///The handle to our open mapping
-    map_handle: HANDLE,
+    /// Pointer to the first byte of our mapping
+    /// Keep this above `file_map` so it gets dropped first
+    pub view: ViewOfFile,
+
+    /// The handle to our open mapping
+    #[allow(dead_code)]
+    file_map: FileMapping,
 
     /// This file is used for shmem persistence. When an owner wants to drop the mapping,
     /// it opens the file with FILE_FLAG_DELETE_ON_CLOSE, renames the file and closes it.
     /// This makes it so future calls to open the old mapping will fail (as it was renamed) and
     /// deletes the renamed file once all handles have been closed.
+    #[allow(dead_code)]
     persistent_file: Option<File>,
 
     //Shared mapping uid
     pub unique_id: String,
     //Total size of the mapping
     pub map_size: usize,
-    //Pointer to the first byte of our mapping
-    pub map_ptr: *mut u8,
 }
 ///Teardown UnmapViewOfFile and close CreateMapping handle
 impl Drop for MapData {
     ///Takes care of properly closing the SharedMem
     fn drop(&mut self) {
-        //Unmap memory from our process
-        if !self.map_ptr.is_null() {
-            trace!("UnmapViewOfFile(map_ptr:{:p})", self.map_ptr);
-            let _ = UnmapViewOfFile(self.map_ptr as _);
-        }
-
-        //Close our mapping
-        if self.map_handle.0 != 0 {
-            trace!("CloseHandle(map_handle:0x{:X})", self.map_handle.0);
-            let _ = CloseHandle(self.map_handle);
-        }
-
         // Inspired by the boost implementation at
         // https://github.com/boostorg/interprocess/blob/140b50efb3281fa3898f3a4cf939cfbda174718f/include/boost/interprocess/detail/win32_api.hpp
         // Emulate POSIX behavior by
@@ -105,6 +95,9 @@ impl MapData {
         self.owner = is_owner;
         prev_val
     }
+    pub fn as_mut_ptr(&self) -> *mut u8 {
+        self.view.as_mut_ptr() as _
+    }
 }
 
 /// Returns the path to a temporary directory in which to store files backing the shared memory. If it
@@ -125,20 +118,13 @@ fn get_tmp_dir() -> Result<PathBuf, ShmemError> {
     }
 }
 
-fn new_map(unique_id: &str, map_size: usize, create: bool)  -> Result<MapData, ShmemError> {
-    let mut f = None;
-    let mut map_h = HANDLE(0);
-
+fn new_map(unique_id: &str, map_size: usize, create: bool) -> Result<MapData, ShmemError> {
     // Create file to back the shared memory
     let mut file_path = get_tmp_dir()?;
     file_path.push(unique_id.trim_start_matches('/'));
     debug!(
         "{} persistent_file at {}",
-        if create {
-            "Creating"
-        } else {
-            "Openning"
-        },
+        if create { "Creating" } else { "Openning" },
         file_path.to_string_lossy()
     );
 
@@ -153,120 +139,108 @@ fn new_map(unique_id: &str, map_size: usize, create: bool)  -> Result<MapData, S
         opt.create(false);
     };
 
-    f = match opt.open(&file_path)
-    {
-        Ok(f) => Some(f),
-        Err(e) if e.kind() == ErrorKind::AlreadyExists => return Err(ShmemError::MappingIdExists),
-        Err(e) => return Err(
-            if create {
-                ShmemError::MapCreateFailed(e.raw_os_error().unwrap() as _)
-            } else {
-                debug!("Failed to open persistent posix mapping... Try to open raw shared memory by ID");
-                // Allow openning memory mappings not managed by this crate
-                // which do not create a file on disk for the mapping
-                ShmemError::MapOpenFailed(e.raw_os_error().unwrap() as _)
-            }),
-    };
+    let mut persistent_file = None;
+    let map_h = match opt.open(&file_path) {
+        Ok(f) => {
+            //Create/open Mapping using persistent file
+            debug!(
+                "{} memory mapping",
+                if create { "Creating" } else { "Openning" },
+            );
+            let high_size: u32 = ((map_size as u64 & 0xFFFF_FFFF_0000_0000_u64) >> 32) as u32;
+            let low_size: u32 = (map_size as u64 & 0xFFFF_FFFF_u64) as u32;
+            trace!(
+                "CreateFileMapping({:?}, NULL, {:X}, {}, {}, '{}')",
+                HANDLE(f.as_raw_handle() as _),
+                PAGE_READWRITE.0,
+                high_size,
+                low_size,
+                unique_id,
+            );
 
-    // Start using MapData ASAP to rely on auto cleanup through Drop
-    let mut new_map: MapData = MapData {
-        owner: create,
-        persistent_file: f,
-        unique_id: String::from(unique_id),
-        map_handle: map_h,
-        map_size,
-        map_ptr: null_mut(),
-    };
-
-    if new_map.map_handle.0 == 0 {
-        //Create Mapping
-        debug!("{} memory mapping",
+            match CreateFileMapping(
+                HANDLE(f.as_raw_handle() as _),
+                None,
+                PAGE_READWRITE,
+                high_size,
+                low_size,
+                unique_id,
+            ) {
+                Ok(v) => {
+                    persistent_file = Some(f);
+                    v
+                }
+                Err(e) => {
+                    let err_code = e.win32_error().unwrap();
+                    return if err_code == ERROR_ALREADY_EXISTS {
+                        Err(ShmemError::MappingIdExists)
+                    } else {
+                        Err(if create {
+                            ShmemError::MapCreateFailed(err_code.0)
+                        } else {
+                            ShmemError::MapOpenFailed(err_code.0)
+                        })
+                    };
+                }
+            }
+        }
+        Err(e) if e.kind() == ErrorKind::AlreadyExists => return Err(ShmemError::MappingIdExists),
+        Err(e) => {
             if create {
-                "Creating"
-            } else {
-                "Openning"
-            },
-        );
-        let high_size: u32 = ((map_size as u64 & 0xFFFF_FFFF_0000_0000_u64) >> 32) as u32;
-        let low_size: u32 = (map_size as u64 & 0xFFFF_FFFF_u64) as u32;
-        trace!(
-            "CreateFileMappingW({:?}, NULL, {:X}, {}, {}, '{}') == 0x{:X}",
-            new_map.persistent_file,
-            PAGE_READWRITE.0,
-            high_size,
-            low_size,
-            new_map.unique_id,
-            new_map.map_handle.0
-        );
+                return Err(ShmemError::MapCreateFailed(e.raw_os_error().unwrap() as _));
+            }
 
-        new_map.map_handle = match CreateFileMappingW(
-            if let Some(h) = &new_map.persistent_file {
-                HANDLE(h.as_raw_handle() as _)
-            } else {
-                INVALID_HANDLE_VALUE
-            },
-            None,
-            PAGE_READWRITE,
-            high_size,
-            low_size,
-            unique_id,
-        ) {
-            Ok(v) => v,
-            Err(e) => {
-                let err_code = e.win32_error().unwrap();
-                return if err_code == ERROR_ALREADY_EXISTS.0 {
-                    Err(ShmemError::MappingIdExists)
-                } else {
-                    Err(if create {
-                        ShmemError::MapCreateFailed(err_code)
-                    } else {
-                        ShmemError::MapOpenFailed(err_code)
-                    })
-                };
+            // This may be a mapping that isnt managed by this crate
+            // Try to open the mapping without any backing file
+            trace!(
+                "OpenFileMappingW({:?}, {}, '{}')",
+                FILE_MAP_ALL_ACCESS,
+                false,
+                unique_id,
+            );
+            match OpenFileMapping(FILE_MAP_ALL_ACCESS, false, unique_id) {
+                Ok(h) => h,
+                Err(e) => {
+                    return Err(ShmemError::MapOpenFailed(e.win32_error().unwrap().0));
+                }
             }
-        };
+        }
+    };
+    trace!("0x{:X}", map_h);
 
-        trace!(
-            "CreateFileMappingW({:?}, NULL, {:X}, {}, {}, '{}') == 0x{:X}",
-            new_map.persistent_file,
-            PAGE_READWRITE.0,
-            high_size,
-            low_size,
-            new_map.unique_id,
-            new_map.map_handle.0
-        );
-    }
-    
     //Map mapping into address space
     debug!("Loading mapping into address space");
     trace!(
         "MapViewOfFile(0x{:X}, {:X}, 0, 0, 0)",
-        new_map.map_handle.0,
+        map_h,
         (FILE_MAP_READ | FILE_MAP_WRITE).0,
     );
-
-    new_map.map_ptr =
-        match MapViewOfFile(new_map.map_handle, FILE_MAP_READ | FILE_MAP_WRITE, 0, 0, 0) {
-            Ok(v) => v as _,
-            Err(e) => return Err(if create {
-                ShmemError::MapCreateFailed(e.win32_error().unwrap())
+    let map_ptr = match MapViewOfFile(map_h.as_handle(), FILE_MAP_READ | FILE_MAP_WRITE, 0, 0, 0) {
+        Ok(v) => v as _,
+        Err(e) => {
+            return Err(if create {
+                ShmemError::MapCreateFailed(e.win32_error().unwrap().0)
             } else {
-                ShmemError::MapOpenFailed(e.win32_error().unwrap())
-            }),
-        };
-    
-    trace!(
-        "MapViewOfFile(0x{:X}, {:X}, 0, 0, 0) == {:p}",
-        new_map.map_handle.0,
-        (FILE_MAP_READ | FILE_MAP_WRITE).0,
-        new_map.map_ptr
-    );
+                ShmemError::MapOpenFailed(e.win32_error().unwrap().0)
+            })
+        }
+    };
+    trace!("\t{:p}", map_ptr);
+
+    let mut new_map = MapData {
+        owner: create,
+        file_map: map_h,
+        persistent_file,
+        unique_id: unique_id.to_string(),
+        map_size: 0,
+        view: map_ptr,
+    };
 
     if !create {
         //Get the real size of the openned mapping
         let mut info = MEMORY_BASIC_INFORMATION::default();
-        if let Err(e) = VirtualQuery(new_map.map_ptr as _, &mut info) {
-            return Err(ShmemError::UnknownOsError(e.win32_error().unwrap()));
+        if let Err(e) = VirtualQuery(new_map.view.as_mut_ptr() as _, &mut info) {
+            return Err(ShmemError::UnknownOsError(e.win32_error().unwrap().0));
         }
         new_map.map_size = info.RegionSize;
     }