Add runtime checks to AioCb methods

Prevent immutable buffers from being used with aio_read or lio_listio
with LIO_READ.  AioCb.from_slice no longer needs to be unsafe.

Author: asomers

src/sys/aio.rs

@@ -62,6 +62,8 @@ pub enum AioCancelStat {
 #[repr(C)]
 pub struct AioCb<'a> {
     aiocb: libc::aiocb,
+    /// Tracks whether the buffer pointed to by aiocb.aio_buf is mutable
+    mutable: bool,
     phantom: PhantomData<&'a mut [u8]>
 }
 
@@ -81,7 +83,7 @@ impl<'a> AioCb<'a> {
         a.aio_nbytes = 0;
         a.aio_buf = null_mut();
 
-        let aiocb = AioCb { aiocb: a, phantom: PhantomData};
+        let aiocb = AioCb { aiocb: a, mutable: false, phantom: PhantomData};
         aiocb
     }
 
@@ -102,37 +104,39 @@ impl<'a> AioCb<'a> {
         let mut a = AioCb::common_init(fd, prio, sigev_notify);
         a.aio_offset = offs;
         a.aio_nbytes = buf.len() as size_t;
+        // casting an immutable buffer to a mutable pointer looks unsafe, but
+        // technically its only unsafe to dereference it, not to create it.
         a.aio_buf = buf.as_ptr() as *mut c_void;
         a.aio_lio_opcode = opcode as ::c_int;
 
-        let aiocb = AioCb { aiocb: a, phantom: PhantomData};
+        let aiocb = AioCb { aiocb: a, mutable: true, phantom: PhantomData};
         aiocb
     }
 
     /// Like `from_mut_slice`, but works on constant slices rather than
     /// mutable slices.
     ///
-    /// This is technically unsafe, but in practice it's fine
-    /// to use with any aio functions except `aio_read` and `lio_listio` (with
-    /// `opcode` set to `LIO_READ`).  This method is useful when writing a const
-    /// buffer with `aio_write`, since from_mut_slice can't work with const
+    /// An `AioCb` created this way cannot be used with `read`, and its
+    /// `LioOpcode` cannot be set to `LIO_READ`.  This method is useful when writing a
+    /// const buffer with `aio_write`, since from_mut_slice can't work with const
     /// buffers.
     // Note: another solution to the problem of writing const buffers would be
     // to genericize AioCb for both &mut [u8] and &[u8] buffers.  aio_read could
     // take the former and aio_write could take the latter.  However, then
     // lio_listio wouldn't work, because that function needs a slice of AioCb,
     // and they must all be the same type.  We're basically stuck with using an
     // unsafe function, since aio (as designed in C) is an unsafe API.
-    pub unsafe fn from_slice(fd: RawFd, offs: off_t, buf: &'a [u8],
-                             prio: ::c_int, sigev_notify: SigevNotify,
-                             opcode: LioOpcode) -> AioCb {
+    pub fn from_slice(fd: RawFd, offs: off_t, buf: &'a [u8],
+                      prio: ::c_int, sigev_notify: SigevNotify,
+                      opcode: LioOpcode) -> AioCb {
         let mut a = AioCb::common_init(fd, prio, sigev_notify);
         a.aio_offset = offs;
         a.aio_nbytes = buf.len() as size_t;
         a.aio_buf = buf.as_ptr() as *mut c_void;
+        assert!(opcode != LioOpcode::LIO_READ, "Can't read into an immutable buffer");
         a.aio_lio_opcode = opcode as ::c_int;
 
-        let aiocb = AioCb { aiocb: a, phantom: PhantomData};
+        let aiocb = AioCb { aiocb: a, mutable: false, phantom: PhantomData};
         aiocb
     }
 
@@ -185,13 +189,15 @@ impl<'a> AioCb<'a> {
 
     /// Asynchronously reads from a file descriptor into a buffer
     pub fn read(&mut self) -> Result<()> {
+        assert!(self.mutable, "Can't read into an immutable buffer");
         let p: *mut libc::aiocb = &mut self.aiocb;
         Errno::result(unsafe { libc::aio_read(p) }).map(drop)
     }
 
     /// Retrieve return status of an asynchronous operation.  Should only be called
     /// once for each `AioCb`, after `aio_error` indicates that it has completed.
     /// The result the same as for `read`, `write`, of `fsync`.
+    // Note: this should be just `return`, but that's a reserved word
     pub fn aio_return(&mut self) -> Result<isize> {
         let p: *mut libc::aiocb = &mut self.aiocb;
         Errno::result(unsafe { libc::aio_return(p) })

test/sys/test_aio.rs

@@ -92,14 +92,12 @@ fn test_aio_suspend() {
     let mut f = tempfile().unwrap();
     f.write(INITIAL).unwrap();
 
-    let mut wcb = unsafe {
-        AioCb::from_slice( f.as_raw_fd(),
+    let mut wcb = AioCb::from_slice( f.as_raw_fd(),
                            2,   //offset
                            &mut WBUF,
                            0,   //priority
                            SigevNotify::SigevNone,
-                           LioOpcode::LIO_WRITE)
-    };
+                           LioOpcode::LIO_WRITE);
 
     let mut rcb = AioCb::from_mut_slice( f.as_raw_fd(),
                             8,   //offset
@@ -150,6 +148,21 @@ fn test_read() {
     assert!(rbuf == EXPECT);
 }
 
+// Test reading into an immutable buffer.  It should fail
+#[test]
+#[should_panic(expected = "Can't read into an immutable buffer")]
+fn test_read_immutable_buffer() {
+    let rbuf = vec![0; 4];
+    let f = tempfile().unwrap();
+    let mut aiocb = AioCb::from_slice( f.as_raw_fd(),
+                           2,   //offset
+                           &rbuf,
+                           0,   //priority
+                           SigevNotify::SigevNone,
+                           LioOpcode::LIO_NOP);
+    aiocb.read().unwrap();
+}
+
 // Test a simple aio operation with no completion notification.  We must poll
 // for completion.  Unlike test_aio_read, this test uses AioCb::from_slice
 #[test]
@@ -161,14 +174,12 @@ fn test_write() {
 
     let mut f = tempfile().unwrap();
     f.write(INITIAL).unwrap();
-    let mut aiocb = unsafe {
-        AioCb::from_slice( f.as_raw_fd(),
+    let mut aiocb = AioCb::from_slice( f.as_raw_fd(),
                            2,   //offset
                            &WBUF,
                            0,   //priority
                            SigevNotify::SigevNone,
-                           LioOpcode::LIO_NOP)
-    };
+                           LioOpcode::LIO_NOP);
     aiocb.write().unwrap();
 
     let err = poll_aio(&mut aiocb);
@@ -206,17 +217,15 @@ fn test_write_sigev_signal() {
 
     let mut f = tempfile().unwrap();
     f.write(INITIAL).unwrap();
-    let mut aiocb = unsafe {
-        AioCb::from_slice( f.as_raw_fd(),
+    let mut aiocb = AioCb::from_slice( f.as_raw_fd(),
                            2,   //offset
                            &WBUF,
                            0,   //priority
                            SigevNotify::SigevSignal {
                                signal: Signal::SIGUSR2,
                                si_value: 0  //TODO: validate in sigfunc
                            },
-                           LioOpcode::LIO_NOP)
-    };
+                           LioOpcode::LIO_NOP);
     aiocb.write().unwrap();
     while unsafe { signaled == 0 } {
         thread::sleep(time::Duration::from_millis(10));
@@ -244,14 +253,12 @@ fn test_lio_listio_wait() {
     f.write(INITIAL).unwrap();
 
     {
-        let mut wcb = unsafe {
-            AioCb::from_slice( f.as_raw_fd(),
+        let mut wcb = AioCb::from_slice( f.as_raw_fd(),
                                2,   //offset
                                &WBUF,
                                0,   //priority
                                SigevNotify::SigevNone,
-                               LioOpcode::LIO_WRITE)
-        };
+                               LioOpcode::LIO_WRITE);
 
         let mut rcb = AioCb::from_mut_slice( f.as_raw_fd(),
                                 8,   //offset
@@ -288,14 +295,12 @@ fn test_lio_listio_nowait() {
     f.write(INITIAL).unwrap();
 
     {
-        let mut wcb = unsafe {
-            AioCb::from_slice( f.as_raw_fd(),
+        let mut wcb = AioCb::from_slice( f.as_raw_fd(),
                                2,   //offset
                                &WBUF,
                                0,   //priority
                                SigevNotify::SigevNone,
-                               LioOpcode::LIO_WRITE)
-        };
+                               LioOpcode::LIO_WRITE);
 
         let mut rcb = AioCb::from_mut_slice( f.as_raw_fd(),
                                 8,   //offset
@@ -339,14 +344,12 @@ fn test_lio_listio_signal() {
     f.write(INITIAL).unwrap();
 
     {
-        let mut wcb = unsafe {
-            AioCb::from_slice( f.as_raw_fd(),
+        let mut wcb = AioCb::from_slice( f.as_raw_fd(),
                                2,   //offset
                                &WBUF,
                                0,   //priority
                                SigevNotify::SigevNone,
-                               LioOpcode::LIO_WRITE)
-        };
+                               LioOpcode::LIO_WRITE);
 
         let mut rcb = AioCb::from_mut_slice( f.as_raw_fd(),
                                 8,   //offset
@@ -372,3 +375,21 @@ fn test_lio_listio_signal() {
     assert!(len == EXPECT.len());
     assert!(rbuf2 == EXPECT);
 }
+
+// Try to use lio_listio to read into an immutable buffer.  It should fail
+#[test]
+#[cfg(not(any(target_os = "ios", target_os = "macos")))]
+#[should_panic(expected = "Can't read into an immutable buffer")]
+fn test_lio_listio_read_immutable() {
+    let rbuf = vec![0; 4];
+    let f = tempfile().unwrap();
+
+
+    let mut rcb = AioCb::from_slice( f.as_raw_fd(),
+                           2,   //offset
+                           &rbuf,
+                           0,   //priority
+                           SigevNotify::SigevNone,
+                           LioOpcode::LIO_READ);
+    let _ = lio_listio(LioMode::LIO_NOWAIT, &[&mut rcb], SigevNotify::SigevNone);
+}