This is an archive of the discontinued Mercurial Phabricator instance.

Differential D12428

rust: explain why the current `OwningDirstateMap` is unsound
ClosedPublic

Authored by Alphare on Apr 4 2022, 11:30 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGcfd270d83169: rust: explain why the current `OwningDirstateMap` is unsound
rHGdf581751909e: rust: explain why the current `OwningDirstateMap` is unsound
Summary

See inline comments.

Diff Detail

Repository
rHG Mercurial
Branch
stable
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

Alphare created this revision.Apr 4 2022, 11:30 AM
baymax updated this revision to Diff 32762.Apr 4 2022, 2:33 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)
⚠ This patch is intended for stable ⚠

Alphare added a commit: rHGdf581751909e: rust: explain why the current `OwningDirstateMap` is unsound.Apr 4 2022, 4:06 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGdf581751909e: rust: explain why the current `OwningDirstateMap` is unsound (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 32748

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

use super::dirstate_map::DirstateMap; use super::dirstate_map::DirstateMap;
use stable_deref_trait::StableDeref; use stable_deref_trait::StableDeref;
use std::ops::Deref; 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 /// Keep a `DirstateMap<'on_disk>` next to the `on_disk` buffer that it
/// borrows. /// borrows.
/// ///
/// This is similar to [`OwningRef`] which is more limited because it /// This is similar to [`OwningRef`] which is more limited because it
/// represents exactly one `&T` reference next to the value it borrows, as /// represents exactly one `&T` reference next to the value it borrows, as
/// opposed to a struct that may contain an arbitrary number of references in /// opposed to a struct that may contain an arbitrary number of references in
/// arbitrarily-nested data structures. /// arbitrarily-nested data structures.
/// ///