From patchwork Mon Apr  4 15:30:24 2022
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: D12428: rust: explain why the current `OwningDirstateMap` is unsound
From: phabricator <phabricator@mercurial-scm.org>
X-Patchwork-Id: 50774
Message-Id: <differential-rev-PHID-DREV-nfsscsrq3atxd35e5t3n-req@mercurial-scm.org>
To: Phabricator <phabricator@mercurial-scm.org>
Cc: mercurial-devel@mercurial-scm.org
Date: Mon, 4 Apr 2022 15:30:24 +0000

Alphare created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REVISION SUMMARY
  See inline comments.

REPOSITORY
  rHG Mercurial

BRANCH
  stable

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

AFFECTED FILES
  rust/hg-core/src/dirstate_tree/owning.rs

CHANGE DETAILS




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

diff --git a/rust/hg-core/src/dirstate_tree/owning.rs b/rust/hg-core/src/dirstate_tree/owning.rs
--- a/rust/hg-core/src/dirstate_tree/owning.rs
+++ b/rust/hg-core/src/dirstate_tree/owning.rs
@@ -2,6 +2,43 @@
 use stable_deref_trait::StableDeref;
 use std::ops::Deref;
 
+/*
+// /!\ This is unsound and can cause use after free. It will be fixed in the
+// next patch
+
+// If we change `value` from its current use of `HgPathBuf` to `&HgPath`,
+// nothing here tells that `value` will outlive `OwningDirstateMap`
+pub fn copy_map_insert<'a,'owned>(
+    &'owned mut self,
+    key: &HgPath,
+    value: &'a HgPath,
+) -> Result<Option<HgPathBuf>, DirstateV2ParseError> {
+    // `'local` is smaller than `'a` here
+    let map: &'local mut DirstateMap<'local> = self.get_map_mut();
+    let node: &'local mut Node<'local> = DirstateMap::get_or_insert_node(
+        map.on_disk,
+        &mut map.unreachable_bytes,
+        &mut map.root,
+        &key,
+        WithBasename::to_cow_owned,
+        |_ancestor| {},
+    )?;
+    if node.copy_source.is_none() {
+        map.nodes_with_copy_source_count += 1
+    }
+    Ok(node.copy_source.replace(value.into()).map(Cow::into_owned))
+    // and right here ----------^^^^^^^^^^^^
+    // we are storing `&'a HgPath` in `Node<'local>` which is possible
+    // because to the compiler, `'a` is longer than ``local`.
+    // It is wrong because nothing proves that `&'a HgPath` will outlive `self`.
+}
+
+// All of this is caused by the wrong cast of the DirstateMap pointer that
+// fakes the lifetime of `DirstateMap` and ensures the compiler that it lives
+// as long as `on_disk`, which is only true for its immutable data.
+// This will be fixed in the next commit.
+*/
+
 /// Keep a `DirstateMap<'on_disk>` next to the `on_disk` buffer that it
 /// borrows.
 ///