Merge pull request #1254 from DanielEScherzer/blame-hunk-null-sig

Avoid null ptr dereferences for `BlameHunk` missing `Signature`s

Author: weihanglo

examples/blame.rs

@@ -81,7 +81,7 @@ fn run(args: &Args) -> Result<(), git2::Error> {
 
     for (i, line) in reader.lines().enumerate() {
         if let (Ok(line), Some(hunk)) = (line, blame.get_line(i + 1)) {
-            let sig = hunk.final_signature();
+            let sig = hunk.final_signature().expect("Should have a signature");
             println!(
                 "{} {} <{}> {}",
                 hunk.final_commit_id(),

src/blame.rs

@@ -106,18 +106,28 @@ impl<'blame> BlameHunk<'blame> {
         unsafe { Oid::from_raw(&(*self.raw).final_commit_id) }
     }
 
-    /// Returns signature for the author of the final commit.
+    /// Returns signature for the author of the final commit, if present.
     ///
     /// The final commit is the one identified by [Self::final_commit_id()].
-    pub fn final_signature(&self) -> Signature<'_> {
-        unsafe { signature::from_raw_const(self, (*self.raw).final_signature) }
+    pub fn final_signature(&self) -> Option<Signature<'_>> {
+        let ptr = unsafe { (*self.raw).final_signature };
+        if ptr.is_null() {
+            None
+        } else {
+            Some(unsafe { signature::from_raw_const(self, ptr) })
+        }
     }
 
-    /// Returns signature for the committer of the final commit.
+    /// Returns signature for the committer of the final commit, if present.
     ///
     /// The final commit is the one identified by [Self::final_commit_id()].
-    pub fn final_committer(&self) -> Signature<'_> {
-        unsafe { signature::from_raw_const(self, (*self.raw).final_committer) }
+    pub fn final_committer(&self) -> Option<Signature<'_>> {
+        let ptr = unsafe { (*self.raw).final_committer };
+        if ptr.is_null() {
+            None
+        } else {
+            Some(unsafe { signature::from_raw_const(self, ptr) })
+        }
     }
 
     /// Returns line number where this hunk begins.
@@ -136,18 +146,28 @@ impl<'blame> BlameHunk<'blame> {
         unsafe { Oid::from_raw(&(*self.raw).orig_commit_id) }
     }
 
-    /// Returns signature of the author of the original commit.
+    /// Returns signature of the author of the original commit, if present.
     ///
     /// The original commit is the one identified by [Self::orig_commit_id()].
-    pub fn orig_signature(&self) -> Signature<'_> {
-        unsafe { signature::from_raw_const(self, (*self.raw).orig_signature) }
+    pub fn orig_signature(&self) -> Option<Signature<'_>> {
+        let ptr = unsafe { (*self.raw).orig_signature };
+        if ptr.is_null() {
+            None
+        } else {
+            Some(unsafe { signature::from_raw_const(self, ptr) })
+        }
     }
 
-    /// Returns signature of the committer of the original commit.
+    /// Returns signature of the committer of the original commit, if present.
     ///
     /// The original commit is the one identified by [Self::orig_commit_id()].
-    pub fn orig_committer(&self) -> Signature<'_> {
-        unsafe { signature::from_raw_const(self, (*self.raw).orig_committer) }
+    pub fn orig_committer(&self) -> Option<Signature<'_>> {
+        let ptr = unsafe { (*self.raw).orig_committer };
+        if ptr.is_null() {
+            None
+        } else {
+            Some(unsafe { signature::from_raw_const(self, ptr) })
+        }
     }
 
     /// Returns line number where this hunk begins.
@@ -411,14 +431,20 @@ mod tests {
 
         let hunk = blame.get_index(0).unwrap();
         assert_eq!(hunk.final_commit_id(), commit);
-        assert_eq!(hunk.final_signature().name(), sig.name());
-        assert_eq!(hunk.final_signature().email(), sig.email());
-        assert_eq!(hunk.orig_signature().name(), sig.name());
-        assert_eq!(hunk.orig_signature().email(), sig.email());
-        assert_eq!(hunk.final_committer().name(), committer_sig.name());
-        assert_eq!(hunk.final_committer().email(), committer_sig.email());
-        assert_eq!(hunk.orig_committer().name(), committer_sig.name());
-        assert_eq!(hunk.orig_committer().email(), committer_sig.email());
+        assert_eq!(hunk.final_signature().unwrap().name(), sig.name());
+        assert_eq!(hunk.final_signature().unwrap().email(), sig.email());
+        assert_eq!(hunk.orig_signature().unwrap().name(), sig.name());
+        assert_eq!(hunk.orig_signature().unwrap().email(), sig.email());
+        assert_eq!(hunk.final_committer().unwrap().name(), committer_sig.name());
+        assert_eq!(
+            hunk.final_committer().unwrap().email(),
+            committer_sig.email()
+        );
+        assert_eq!(hunk.orig_committer().unwrap().name(), committer_sig.name());
+        assert_eq!(
+            hunk.orig_committer().unwrap().email(),
+            committer_sig.email()
+        );
         assert_eq!(hunk.final_start_line(), 1);
         assert_eq!(hunk.path(), Some(Path::new("foo/bar")));
         assert_eq!(hunk.lines_in_hunk(), 0);
@@ -432,4 +458,89 @@ mod tests {
         assert_eq!(blame_buffer.iter().count(), 2);
         assert!(line.final_commit_id().is_zero());
     }
+
+    #[test]
+    fn buffer_signatures() {
+        // Regression tests for #1253
+        let td = tempfile::TempDir::new().unwrap();
+        let path = td.path();
+
+        let repo = crate::Repository::init(path).unwrap();
+
+        {
+            let mut config = repo.config().unwrap();
+            config.set_str("user.name", "name").unwrap();
+            config.set_str("user.email", "email").unwrap();
+
+            fs::write(&path.join("README.md"), "Testing").unwrap();
+
+            let mut index = repo.index().unwrap();
+            index.add_path(&Path::new("README.md")).unwrap();
+            index.write().unwrap();
+
+            let id = index.write_tree().unwrap();
+            let tree = repo.find_tree(id).unwrap();
+            let sig = repo.signature().unwrap();
+            repo.commit(Some("HEAD"), &sig, &sig, "Add README.md", &tree, &[])
+                .unwrap();
+        }
+
+        let blame = repo.blame_file(&Path::new("README.md"), None).unwrap();
+        // This hunk is safe to use
+        let hunk = blame.get_index(0).unwrap();
+
+        {
+            let final_author = hunk.final_signature().unwrap();
+            assert_eq!(Some("name"), final_author.name());
+            assert_eq!(Some("email"), final_author.email());
+
+            let final_committer = hunk.final_committer().unwrap();
+            assert_eq!(Some("name"), final_committer.name());
+            assert_eq!(Some("email"), final_committer.email());
+
+            let original_author = hunk.orig_signature().unwrap();
+            assert_eq!(Some("name"), original_author.name());
+            assert_eq!(Some("email"), original_author.email());
+
+            let original_committer = hunk.orig_committer().unwrap();
+            assert_eq!(Some("name"), original_committer.name());
+            assert_eq!(Some("email"), original_committer.email());
+        }
+
+        let arbitrary = blame.blame_buffer(b"abc123").unwrap();
+        let hunk = arbitrary.get_index(0).unwrap();
+        // This hunk is NOT safe to use
+        // the final_signature, final_committer, orig_signature, and
+        // orig_committer pointers are all NULL
+        // But the other methods still work
+        {
+            let final_commit_id = hunk.final_commit_id();
+            assert!(final_commit_id.is_zero());
+
+            let original_commit_id = hunk.orig_commit_id();
+            assert!(original_commit_id.is_zero());
+
+            assert_eq!(1, hunk.final_start_line());
+            assert_eq!(0, hunk.orig_start_line());
+            assert_eq!(Some(Path::new("README.md")), hunk.path());
+            assert_eq!(false, hunk.is_boundary());
+            assert_eq!(1, hunk.lines_in_hunk());
+            assert_eq!(None, hunk.summary());
+            assert_eq!(None, hunk.summary_bytes());
+        }
+
+        {
+            let final_author = hunk.final_signature();
+            assert!(final_author.is_none());
+
+            let final_committer = hunk.final_committer();
+            assert!(final_committer.is_none());
+
+            let original_author = hunk.orig_signature();
+            assert!(original_author.is_none());
+
+            let original_committer = hunk.orig_committer();
+            assert!(original_committer.is_none());
+        }
+    }
 }