Allocate guard page under existing stack, not in it.

This fixes a segfault when the allocated stack is just one page long.

This also refactors the fringe::os module to use Result consistently.

close #22

Cargo.toml

@@ -4,7 +4,7 @@ name = "fringe"
 version = "0.0.1"
 
 [target.'cfg(unix)'.dependencies]
-libc = "0.1.6"
+libc = "0.2.14"
 
 [target.'cfg(windows)'.dependencies]
 kernel32-sys = "0.2.2"

src/os/mod.rs

@@ -1,10 +1,10 @@
 // This file is part of libfringe, a low-level green threading library.
 // Copyright (c) edef <edef@edef.eu>
 // See the LICENSE file included in this distribution.
-
 extern crate std;
 use self::std::io::Error as IoError;
 use stack;
+
 mod sys;
 
 /// This object represents a stack from the operating system's
@@ -23,24 +23,22 @@ impl Stack {
   pub fn new(size: usize) -> Result<Stack, IoError> {
     let page_size = sys::page_size();
 
-    // round the page size up,
-    // using the fact that it is a power of two
+    // Round the length one page size up, using the fact that the page size
+    // is a power of two.
     let len = (size + page_size - 1) & !(page_size - 1);
 
-    let stack = unsafe {
-      let ptr = try!(match sys::map_stack(size) {
-        None => Err(IoError::last_os_error()),
-        Some(ptr) => Ok(ptr)
-      });
+    // Increase the length to fit the guard page.
+    let len = len + page_size;
 
-      Stack { ptr: ptr as *mut u8, len: len }
+    // Allocate a stack.
+    let stack = Stack {
+      ptr: try!(unsafe { sys::map_stack(len) }),
+      len: len
     };
 
-    try!(if unsafe { sys::protect_stack(stack.ptr) } {
-      Ok(())
-    } else {
-      Err(IoError::last_os_error())
-    });
+    // Mark the guard page. If this fails, `stack` will be dropped,
+    // unmapping it.
+    try!(unsafe { sys::protect_stack(stack.ptr) });
 
     Ok(stack)
   }
@@ -62,11 +60,6 @@ impl stack::Stack for Stack {
 
 impl Drop for Stack {
   fn drop(&mut self) {
-    unsafe {
-      if !sys::unmap_stack(self.ptr, self.len) {
-        panic!("munmap for stack {:p} of size {} failed: {}",
-               self.ptr, self.len, IoError::last_os_error())
-      }
-    }
+    unsafe { sys::unmap_stack(self.ptr, self.len) }.expect("cannot unmap stack")
   }
 }

src/os/sys/unix.rs

@@ -1,7 +1,9 @@
 // This file is part of libfringe, a low-level green threading library.
 // Copyright (c) edef <edef@edef.eu>
 // See the LICENSE file included in this distribution.
+extern crate std;
 extern crate libc;
+use self::std::io::Error as IoError;
 use self::libc::{c_void, c_int, size_t};
 use self::libc::{mmap, mprotect, munmap};
 use self::libc::MAP_FAILED;
@@ -29,20 +31,27 @@ const STACK_FLAGS: c_int = libc::MAP_STACK
 const STACK_FLAGS: c_int = libc::MAP_PRIVATE
                          | libc::MAP_ANON;
 
-pub unsafe fn map_stack(len: usize) -> Option<*mut u8> {
-  let ptr = mmap(ptr::null_mut(), len as size_t,
-                 STACK_PROT, STACK_FLAGS, -1, 0);
+pub unsafe fn map_stack(len: usize) -> Result<*mut u8, IoError> {
+  let ptr = mmap(ptr::null_mut(), len as size_t, STACK_PROT, STACK_FLAGS, -1, 0);
   if ptr == MAP_FAILED {
-    None
+    Err(IoError::last_os_error())
   } else {
-    Some(ptr as *mut u8)
+    Ok(ptr as *mut u8)
   }
 }
 
-pub unsafe fn protect_stack(ptr: *mut u8) -> bool {
-  mprotect(ptr as *mut c_void, page_size() as size_t, GUARD_PROT) == 0
+pub unsafe fn protect_stack(ptr: *mut u8) -> Result<(), IoError> {
+  if mprotect(ptr as *mut c_void, page_size() as size_t, GUARD_PROT) == 0 {
+    Ok(())
+  } else {
+    Err(IoError::last_os_error())
+  }
 }
 
-pub unsafe fn unmap_stack(ptr: *mut u8, len: usize) -> bool {
-  munmap(ptr as *mut c_void, len as size_t) == 0
+pub unsafe fn unmap_stack(ptr: *mut u8, len: usize) -> Result<(), IoError> {
+  if munmap(ptr as *mut c_void, len as size_t) == 0 {
+    Ok(())
+  } else {
+    Err(IoError::last_os_error())
+  }
 }

src/os/sys/windows.rs

@@ -1,15 +1,17 @@
 // This file is part of libfringe, a low-level green threading library.
 // Copyright (c) edef <edef@edef.eu>
 // See the LICENSE file included in this distribution.
+extern crate std;
 extern crate winapi;
 extern crate kernel32;
-use core::{mem, ptr};
+use self::std::io::Error as IoError;
+use self::std::{mem, ptr};
 use self::winapi::basetsd::SIZE_T;
 use self::winapi::minwindef::{DWORD, LPVOID};
 use self::winapi::winnt::{MEM_COMMIT, MEM_RESERVE, MEM_RELEASE, PAGE_READWRITE, PAGE_NOACCESS};
 use super::page_size;
 
-#[cfg(windows)]
+#[cold]
 pub fn sys_page_size() -> usize {
   unsafe {
     let mut info = mem::zeroed();
@@ -18,20 +20,30 @@ pub fn sys_page_size() -> usize {
   }
 }
 
-pub unsafe fn map_stack(len: usize) -> Option<*mut u8> {
-  let ptr = kernel32::VirtualAlloc(ptr::null_mut(), len as SIZE_T, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
+pub unsafe fn map_stack(len: usize) -> Result<*mut u8, IoError> {
+  let ptr = kernel32::VirtualAlloc(ptr::null_mut(), len as SIZE_T,
+                                   MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
   if ptr == ptr::null_mut() {
-    None
+    Err(IoError::last_os_error())
   } else {
-    Some(ptr as *mut u8)
+    Ok(ptr as *mut u8)
   }
 }
 
-pub unsafe fn protect_stack(ptr: *mut u8) -> bool {
+pub unsafe fn protect_stack(ptr: *mut u8) -> Result<(), IoError> {
   let mut old_prot: DWORD = 0;
-  kernel32::VirtualProtect(ptr as LPVOID, page_size() as SIZE_T, PAGE_NOACCESS, &mut old_prot) != 0
+  if kernel32::VirtualProtect(ptr as LPVOID, page_size() as SIZE_T,
+                              PAGE_NOACCESS, &mut old_prot) == 0 {
+    Err(IoError::last_os_error())
+  } else {
+    Ok(())
+  }
 }
 
-pub unsafe fn unmap_stack(ptr: *mut u8, _len: usize) -> bool {
-  kernel32::VirtualFree(ptr as LPVOID, 0, MEM_RELEASE) != 0
+pub unsafe fn unmap_stack(ptr: *mut u8, _len: usize) -> Result<(), IoError> {
+  if kernel32::VirtualFree(ptr as LPVOID, 0, MEM_RELEASE) == 0 {
+    Err(IoError::last_os_error())
+  } else {
+    Ok(())
+  }
 }

tests/stack.rs

@@ -0,0 +1,13 @@
+// This file is part of libfringe, a low-level green threading library.
+// Copyright (c) whitequark <whitequark@whitequark.org>
+// See the LICENSE file included in this distribution.
+extern crate fringe;
+
+use fringe::{Stack, OsStack};
+
+#[test]
+fn stack_accessible() {
+  let stack = OsStack::new(4096).unwrap();
+  // Make sure the topmost page of the stack, at least, is accessible.
+  unsafe { *(stack.top().offset(-1)) = 0; }
+}