Corrected undefined behaviour when calling Trajectory.read() on improperly sized Frame

src/errors.rs

@@ -22,8 +22,9 @@ pub enum ErrorTask {
 #[derive(Debug, Clone, PartialEq)]
 /// Error type for the xdrfile library
 pub struct Error {
-    code: Option<ErrorCode>,
+    code: ErrorCode,
     task: ErrorTask,
+    source: Option<Box<Error>>,
 }
 
 impl Error {
@@ -33,60 +34,75 @@ impl Error {
     }
 
     /// Get the error code returned by the C API, if any
-    pub fn code(&self) -> &Option<ErrorCode> {
+    pub fn code(&self) -> &ErrorCode {
         &self.code
     }
 
     /// True if the error is an end of file error, false otherwise
     pub fn is_eof(&self) -> bool {
-        self.code.as_ref().map_or(false, ErrorCode::is_eof)
+        self.code.is_eof()
     }
 
     /// Construct a new error during the ToCString task
     pub(crate) fn from_convert() -> Self {
         Self {
-            code: None,
+            code: ErrorCode::NoCode,
             task: ErrorTask::ToCString(None),
+            source: None,
         }
     }
 
     /// Construct a new error during the OpenFile task
     pub(crate) fn from_open(path: impl AsRef<Path>, mode: FileMode) -> Self {
         Self {
-            code: None,
+            code: ErrorCode::NoCode,
             task: ErrorTask::OpenFile(path.as_ref().into(), mode),
+            source: None,
         }
     }
 
     /// Construct a new error during the ReadNumAtoms task from a C error code
     pub(crate) fn from_read_num_atoms(code: impl Into<ErrorCode>) -> Self {
         Self {
-            code: Some(code.into()),
+            code: code.into(),
             task: ErrorTask::ReadNumAtoms,
+            source: None,
         }
     }
 
     /// Construct a new error during the Read task from a C error code
     pub(crate) fn from_read(code: impl Into<ErrorCode>) -> Self {
         Self {
-            code: Some(code.into()),
+            code: code.into(),
             task: ErrorTask::Read,
+            source: None,
         }
     }
 
     /// Construct a new error during the Write task from a C error code
     pub(crate) fn from_write(code: impl Into<ErrorCode>) -> Self {
         Self {
-            code: Some(code.into()),
+            code: code.into(),
             task: ErrorTask::Write,
+            source: None,
         }
     }
 
     /// Construct a new error during the Flush task from a C error code
     pub(crate) fn from_flush(code: impl Into<ErrorCode>) -> Self {
         Self {
-            code: Some(code.into()),
+            code: code.into(),
             task: ErrorTask::Flush,
+            source: None,
+        }
+    }
+
+    /// Construct a new error during the ReadNumAtoms task from a C error code
+    pub(crate) fn with_task(self, task: ErrorTask) -> Self {
+        Self {
+            code: self.code,
+            task,
+            source: Some(Box::new(self)),
         }
     }
 }
@@ -94,8 +110,9 @@ impl Error {
 impl From<std::ffi::NulError> for Error {
     fn from(err: std::ffi::NulError) -> Self {
         Self {
-            code: None,
+            code: ErrorCode::NoCode,
             task: ErrorTask::ToCString(Some(err)),
+            source: None,
         }
     }
 }
@@ -128,8 +145,10 @@ impl std::fmt::Display for Error {
 
 impl std::error::Error for Error {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
-        if let Some(e) = &self.code {
-            Some(e)
+        if let Some(source) = &self.source {
+            Some(source.as_ref())
+        } else if self.code != ErrorCode::NoCode {
+            Some(&self.code)
         } else if let ErrorTask::ToCString(Some(e)) = &self.task {
             Some(e)
         } else {
@@ -171,6 +190,8 @@ pub enum ErrorCode {
     ExdrNr,
     /// Something unexpected happened
     UnmatchedCode(c_abi::xdrfile::BindgenTy1),
+    /// C returned no code at all
+    NoCode,
 }
 
 impl ErrorCode {
@@ -242,38 +263,44 @@ mod tests {
     #[test]
     fn test_is_eof() {
         let error = Error {
-            code: Some(c_abi::xdrfile::exdrENDOFFILE.into()),
+            code: c_abi::xdrfile::exdrENDOFFILE.into(),
             task: ErrorTask::Read,
+            source: None,
         };
         assert!(error.is_eof());
 
         let error = Error {
-            code: Some(ErrorCode::ExdrEndOfFile),
+            code: ErrorCode::ExdrEndOfFile,
             task: ErrorTask::Read,
+            source: None,
         };
         assert!(error.is_eof());
 
         let error = Error {
-            code: Some((c_abi::xdrfile::exdrENDOFFILE + 1).into()),
+            code: (c_abi::xdrfile::exdrENDOFFILE + 1).into(),
             task: ErrorTask::Read,
+            source: None,
         };
         assert!(!error.is_eof());
 
         let error = Error {
-            code: Some(0.into()),
+            code: 0.into(),
             task: ErrorTask::Write,
+            source: None,
         };
         assert!(!error.is_eof());
 
         let error = Error {
-            code: Some(255.into()),
+            code: 255.into(),
             task: ErrorTask::Flush,
+            source: None,
         };
         assert!(!error.is_eof());
 
         let error = Error {
-            code: None,
+            code: ErrorCode::NoCode,
             task: ErrorTask::OpenFile(PathBuf::from("not/a/file"), FileMode::Read),
+            source: None,
         };
         assert!(!error.is_eof());
     }

src/frame.rs

@@ -1,7 +1,7 @@
 use std::fmt;
 
 /// A frame represents a single step in a trajectory.
-#[derive(Clone)]
+#[derive(Clone, PartialEq)]
 pub struct Frame {
     /// Number of atoms in the frame
     pub num_atoms: u32,
@@ -13,10 +13,10 @@ pub struct Frame {
     pub time: f32,
 
     /// 3x3 box vector
-    pub box_vector: [[f32; 3usize]; 3usize],
+    pub box_vector: [[f32; 3]; 3],
 
     /// 3D coordinates for N atoms where N is num_atoms
-    pub coords: Vec<[f32; 3usize]>,
+    pub coords: Vec<[f32; 3]>,
 }
 
 impl Default for Frame {
@@ -76,6 +76,12 @@ impl Frame {
     pub fn len(self: &Frame) -> usize {
         self.num_atoms as usize
     }
+
+    /// Resizes the Frame in-place so that num_atoms is equal to new_size.
+    pub fn resize(&mut self, new_size: u32) {
+        self.num_atoms = new_size;
+        self.coords.resize(new_size as usize, [0.0; 3]);
+    }
 }
 
 #[cfg(test)]

src/lib.rs

@@ -201,6 +201,10 @@ impl XTCTrajectory {
 impl Trajectory for XTCTrajectory {
     fn read(&mut self, frame: &mut Frame) -> Result<()> {
         let mut step: i32 = 0;
+        frame.resize(
+            self.get_num_atoms()
+                .map_err(|e| e.with_task(ErrorTask::Read))?,
+        );
         unsafe {
             // C lib requires an i32 to be passed, but step is exposed it as u32
             // (A step cannot be negative, can it?). So we need to create a step
@@ -296,6 +300,10 @@ impl Trajectory for TRRTrajectory {
     fn read(&mut self, frame: &mut Frame) -> Result<()> {
         let mut step: i32 = 0;
         let mut lambda: f32 = 0.0;
+        frame.resize(
+            self.get_num_atoms()
+                .map_err(|e| e.with_task(ErrorTask::Read))?,
+        );
         unsafe {
             // C lib requires an i32 to be passed, but step is exposed it as u32
             // (A step cannot be negative, can it?). So we need to create a step
@@ -446,6 +454,38 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    pub fn test_manual_loop() -> Result<(), Box<dyn std::error::Error>> {
+        let mut frame = Frame::new();
+
+        let mut xtc_frames = Vec::new();
+        let mut xtc_traj = XTCTrajectory::open_read("tests/1l2y.xtc")?;
+
+        while let Ok(()) = xtc_traj.read(&mut frame) {
+            xtc_frames.push(frame.clone());
+        }
+
+        let mut trr_frames = Vec::new();
+        let mut trr_traj = TRRTrajectory::open_read("tests/1l2y.trr")?;
+
+        while let Ok(()) = trr_traj.read(&mut frame) {
+            trr_frames.push(frame.clone());
+        }
+
+        for (xtc, trr) in xtc_frames.into_iter().zip(trr_frames) {
+            assert_eq!(xtc.num_atoms, trr.num_atoms);
+            assert_eq!(xtc.step, trr.step);
+            assert_eq!(xtc.time, trr.time);
+            assert_eq!(xtc.box_vector, trr.box_vector);
+            for (xtc_xyz, trr_xyz) in xtc.coords.into_iter().zip(trr.coords) {
+                assert!(xtc_xyz[0] - trr_xyz[0] <= 1e-5);
+                assert!(xtc_xyz[1] - trr_xyz[1] <= 1e-5);
+                assert!(xtc_xyz[2] - trr_xyz[2] <= 1e-5);
+            }
+        }
+        Ok(())
+    }
+
     #[test]
     fn test_path_to_cstring() -> Result<(), Box<dyn std::error::Error>> {
         let result_invalid = path_to_cstring(PathBuf::from("invalid/\0path"));
@@ -493,7 +533,7 @@ mod tests {
         if let Err(e) = trr.get_num_atoms() {
             match e.task() {
                 ErrorTask::ReadNumAtoms => {
-                    assert_eq!(Some(ErrorCode::ExdrMagic), *e.code());
+                    assert_eq!(ErrorCode::ExdrMagic, *e.code());
                 }
                 _ => panic!("Wrong Error type"),
             }
@@ -509,7 +549,7 @@ mod tests {
         if let Err(e) = trr.read(&mut frame) {
             match e.task() {
                 ErrorTask::Read => {
-                    assert_eq!(Some(ErrorCode::ExdrMagic), *e.code());
+                    assert_eq!(ErrorCode::ExdrMagic, *e.code());
                 }
                 _ => panic!("Wrong Error type"),
             }