Patchwork D11882: rhg: fix a crash on non-generaldelta revlogs

login
register
mail settings
Submitter phabricator
Date Dec. 7, 2021, 6:58 p.m.
Message ID <differential-rev-PHID-DREV-dpqvbgby2j2dwfq6vkia-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/50208/
State New
Headers show

Comments

phabricator - Dec. 7, 2021, 6:58 p.m.
aalekseyev created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REPOSITORY
  rHG Mercurial

BRANCH
  stable

REVISION DETAIL
  https://phab.mercurial-scm.org/D11882

AFFECTED FILES
  rust/hg-core/src/revlog/index.rs
  rust/hg-core/src/revlog/revlog.rs
  tests/test-rhg-no-generaldelta.t

CHANGE DETAILS




To: aalekseyev, #hg-reviewers
Cc: mercurial-patches, mercurial-devel

Patch

diff --git a/tests/test-rhg-no-generaldelta.t b/tests/test-rhg-no-generaldelta.t
--- a/tests/test-rhg-no-generaldelta.t
+++ b/tests/test-rhg-no-generaldelta.t
@@ -22,9 +22,7 @@ 
         1       1        2        0    prev         14        143         93   0.65035        93         0    0.00000
         2       1        3        1    prev         12        141        105   0.74468       105         0    0.00000
 
-rhg breaks on non-generaldelta revlogs:
+rhg works on non-generaldelta revlogs:
 
   $ $NO_FALLBACK hg cat f -r . | f --sha256 --size
-  abort: corrupted revlog (rhg !)
-  size=0, sha256=e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 (rhg !)
-  size=141, sha256=1a7fe778e33d64d5e14a9a126b77038b328356e67bacf308797bc0e39bf204f3 (no-rhg !)
+  size=141, sha256=1a7fe778e33d64d5e14a9a126b77038b328356e67bacf308797bc0e39bf204f3
diff --git a/rust/hg-core/src/revlog/revlog.rs b/rust/hg-core/src/revlog/revlog.rs
--- a/rust/hg-core/src/revlog/revlog.rs
+++ b/rust/hg-core/src/revlog/revlog.rs
@@ -191,11 +191,20 @@ 
         // Todo return -> Cow
         let mut entry = self.get_entry(rev)?;
         let mut delta_chain = vec![];
-        while let Some(base_rev) = entry.base_rev {
-            delta_chain.push(entry);
-            entry = self
-                .get_entry(base_rev)
-                .map_err(|_| RevlogError::corrupted())?;
+
+        if self.index.uses_generaldelta() {
+            while let Some(base_rev) = entry.base_rev_or_base_of_delta_chain {
+                delta_chain.push(entry);
+                entry = self.get_entry_internal(base_rev)?;
+            }
+        } else {
+            if let Some(base_rev) = entry.base_rev_or_base_of_delta_chain {
+                delta_chain.push(entry);
+                entry = self.get_entry_internal(base_rev)?;
+                for rev in (base_rev + 1..rev).rev() {
+                    delta_chain.push(self.get_entry_internal(rev)?);
+                }
+            }
         }
 
         // TODO do not look twice in the index
@@ -291,14 +300,26 @@ 
             bytes: data,
             compressed_len: index_entry.compressed_len(),
             uncompressed_len: index_entry.uncompressed_len(),
-            base_rev: if index_entry.base_revision() == rev {
+            base_rev_or_base_of_delta_chain: if index_entry
+                .base_revision_or_base_of_delta_chain()
+                == rev
+            {
                 None
             } else {
-                Some(index_entry.base_revision())
+                Some(index_entry.base_revision_or_base_of_delta_chain())
             },
         };
         Ok(entry)
     }
+
+    // when resolving internal references within revlog, any errors
+    // should be reported as corruption, instead of e.g. "invalid revision"
+    fn get_entry_internal(
+        &self,
+        rev: Revision,
+    ) -> Result<RevlogEntry, RevlogError> {
+        return self.get_entry(rev).map_err(|_| RevlogError::corrupted());
+    }
 }
 
 /// The revlog entry's bytes and the necessary informations to extract
@@ -309,7 +330,7 @@ 
     bytes: &'a [u8],
     compressed_len: usize,
     uncompressed_len: usize,
-    base_rev: Option<Revision>,
+    base_rev_or_base_of_delta_chain: Option<Revision>,
 }
 
 impl<'a> RevlogEntry<'a> {
@@ -375,7 +396,7 @@ 
     /// Tell if the entry is a snapshot or a delta
     /// (influences on decompression).
     fn is_delta(&self) -> bool {
-        self.base_rev.is_some()
+        self.base_rev_or_base_of_delta_chain.is_some()
     }
 }
 
diff --git a/rust/hg-core/src/revlog/index.rs b/rust/hg-core/src/revlog/index.rs
--- a/rust/hg-core/src/revlog/index.rs
+++ b/rust/hg-core/src/revlog/index.rs
@@ -85,6 +85,7 @@ 
     /// Offsets of starts of index blocks.
     /// Only needed when the index is interleaved with data.
     offsets: Option<Vec<usize>>,
+    uses_generaldelta: bool,
 }
 
 impl Index {
@@ -101,6 +102,11 @@ 
             return Err(HgError::corrupted("unsupported revlog version"));
         }
 
+        // This is only correct because we know version is REVLOGV1.
+        // In v2 we always use generaldelta, while in v0 we never use
+        // generaldelta. Similar for [is_inline] (it's only used in v1).
+        let uses_generaldelta = header.format_flags().uses_generaldelta();
+
         if header.format_flags().is_inline() {
             let mut offset: usize = 0;
             let mut offsets = Vec::new();
@@ -120,6 +126,7 @@ 
                 Ok(Self {
                     bytes,
                     offsets: Some(offsets),
+                    uses_generaldelta,
                 })
             } else {
                 Err(HgError::corrupted("unexpected inline revlog length")
@@ -129,11 +136,17 @@ 
             Ok(Self {
                 bytes,
                 offsets: None,
+                uses_generaldelta,
             })
         }
     }
 
     /// Value of the inline flag.
+    pub fn uses_generaldelta(&self) -> bool {
+        self.uses_generaldelta
+    }
+
+    /// Value of the inline flag.
     pub fn is_inline(&self) -> bool {
         self.offsets.is_some()
     }
@@ -260,7 +273,7 @@ 
     }
 
     /// Return the revision upon which the data has been derived.
-    pub fn base_revision(&self) -> Revision {
+    pub fn base_revision_or_base_of_delta_chain(&self) -> Revision {
         // TODO Maybe return an Option when base_revision == rev?
         //      Requires to add rev to IndexEntry
 
@@ -298,7 +311,7 @@ 
         offset: usize,
         compressed_len: usize,
         uncompressed_len: usize,
-        base_revision: Revision,
+        base_revision_or_base_of_delta_chain: Revision,
     }
 
     #[cfg(test)]
@@ -312,7 +325,7 @@ 
                 offset: 0,
                 compressed_len: 0,
                 uncompressed_len: 0,
-                base_revision: 0,
+                base_revision_or_base_of_delta_chain: 0,
             }
         }
 
@@ -351,8 +364,11 @@ 
             self
         }
 
-        pub fn with_base_revision(&mut self, value: Revision) -> &mut Self {
-            self.base_revision = value;
+        pub fn with_base_revision_or_base_of_delta_chain(
+            &mut self,
+            value: Revision,
+        ) -> &mut Self {
+            self.base_revision_or_base_of_delta_chain = value;
             self
         }
 
@@ -375,7 +391,9 @@ 
             bytes.extend(&[0u8; 2]); // Revision flags.
             bytes.extend(&(self.compressed_len as u32).to_be_bytes());
             bytes.extend(&(self.uncompressed_len as u32).to_be_bytes());
-            bytes.extend(&self.base_revision.to_be_bytes());
+            bytes.extend(
+                &self.base_revision_or_base_of_delta_chain.to_be_bytes(),
+            );
             bytes
         }
     }
@@ -458,14 +476,16 @@ 
     }
 
     #[test]
-    fn test_base_revision() {
-        let bytes = IndexEntryBuilder::new().with_base_revision(1).build();
+    fn test_base_revision_or_base_of_delta_chain() {
+        let bytes = IndexEntryBuilder::new()
+            .with_base_revision_or_base_of_delta_chain(1)
+            .build();
         let entry = IndexEntry {
             bytes: &bytes,
             offset_override: None,
         };
 
-        assert_eq!(entry.base_revision(), 1)
+        assert_eq!(entry.base_revision_or_base_of_delta_chain(), 1)
     }
 }