This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11089

dirstate-v2: Enforce data size read from the docket file
ClosedPublic

Authored by SimonSapin on Jul 12 2021, 5:10 PM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG48aec076b8fb: dirstate-v2: Enforce data size read from the docket file
Summary

The data file may not be shorter than its size given by the docket.
It may be longer, but additional data is ignored.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

SimonSapin created this revision.Jul 12 2021, 5:10 PM
baymax updated this revision to Diff 29219.Jul 12 2021, 6:39 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

Alphare accepted this revision.Jul 13 2021, 11:03 AM
This revision is now accepted and ready to land.Jul 13 2021, 11:03 AM
SimonSapin updated this revision to Diff 29236.Jul 13 2021, 3:50 PM
marmoute added a subscriber: marmoute.Jul 16 2021, 5:53 AM

Either I am missing something or we could so something much simpler here.

mercurial/dirstatemap.py
637–643

Why dont' we simply ready docket.data_size data from disk ? instead of doing later filtering ?

SimonSapin added inline comments.Jul 16 2021, 8:13 AM
mercurial/dirstatemap.py
637–643

Yeah that should work, I didn’t think of that. I went with doing the slicing on the Rust side because slicing a Python bytes object allocates a new one and copies. I don’t know how that will work when we use mmap. It’ll probably involve a memoryview?

baymax updated this revision to Diff 29607.Jul 20 2021, 3:41 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

Revision Contents
Changeset List

Diff 29618

mercurial/dirstatemap.py

# ignore HG_PENDING because identity is used only for writing # ignore HG_PENDING because identity is used only for writing
self.identity = util.filestat.frompath( self.identity = util.filestat.frompath(
self._opener.join(self._filename) self._opener.join(self._filename)
) )
if self._use_dirstate_v2: if self._use_dirstate_v2:
if self.docket.uuid: if self.docket.uuid:
# TODO: use mmap when possible # TODO: use mmap when possible
data = self._opener.read(self.docket.data_filename()) data = self._opener.read(self.docket.data_filename())
else: else:
data = b'' data = b''
self._rustmap = rustmod.DirstateMap.new_v2(data) self._rustmap = rustmod.DirstateMap.new_v2(
data, self.docket.data_size
)
parents = self.docket.parents parents = self.docket.parents
marmouteUnsubmitted
Not Done

Why dont' we simply ready docket.data_size data from disk ? instead of doing later filtering ?

marmoute: Why dont' we simply ready `docket.data_size` data from disk ? instead of doing later filtering ?
SimonSapinAuthorUnsubmitted
Done

Yeah that should work, I didn’t think of that. I went with doing the slicing on the Rust side because slicing a Python bytes object allocates a new one and copies. I don’t know how that will work when we use mmap. It’ll probably involve a memoryview?

SimonSapin: Yeah that should work, I didn’t think of that. I went with doing the slicing on the Rust side…
else: else:
self._rustmap, parents = rustmod.DirstateMap.new_v1( self._rustmap, parents = rustmod.DirstateMap.new_v1(
self._use_dirstate_tree, self._readdirstatefile() self._use_dirstate_tree, self._readdirstatefile()
) )
if parents and not self._dirtyparents: if parents and not self._dirtyparents:
self.setparents(*parents) self.setparents(*parents)

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

root: ChildNodes::default(), root: ChildNodes::default(),
nodes_with_entry_count: 0, nodes_with_entry_count: 0,
nodes_with_copy_source_count: 0, nodes_with_copy_source_count: 0,
ignore_patterns_hash: [0; on_disk::IGNORE_PATTERNS_HASH_LEN], ignore_patterns_hash: [0; on_disk::IGNORE_PATTERNS_HASH_LEN],
} }
} }
#[timed] #[timed]
pub fn new_v2(on_disk: &'on_disk [u8]) -> Result<Self, DirstateError> { pub fn new_v2(
Ok(on_disk::read(on_disk)?) on_disk: &'on_disk [u8],
data_size: usize,
) -> Result<Self, DirstateError> {
if let Some(data) = on_disk.get(..data_size) {
Ok(on_disk::read(data)?)
} else {
Err(DirstateV2ParseError.into())
}
} }
#[timed] #[timed]
pub fn new_v1( pub fn new_v1(
on_disk: &'on_disk [u8], on_disk: &'on_disk [u8],
) -> Result<(Self, Option<DirstateParents>), DirstateError> { ) -> Result<(Self, Option<DirstateParents>), DirstateError> {
let mut map = Self::empty(on_disk); let mut map = Self::empty(on_disk);
if map.on_disk.is_empty() { if map.on_disk.is_empty() {

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

use crate::DirstateEntry; use crate::DirstateEntry;
use crate::DirstateError; use crate::DirstateError;
use crate::DirstateParents; use crate::DirstateParents;
use crate::EntryState; use crate::EntryState;
use bytes_cast::unaligned::{I32Be, I64Be, U32Be}; use bytes_cast::unaligned::{I32Be, I64Be, U32Be};
use bytes_cast::BytesCast; use bytes_cast::BytesCast;
use format_bytes::format_bytes; use format_bytes::format_bytes;
use std::borrow::Cow; use std::borrow::Cow;
use std::convert::TryFrom; use std::convert::{TryFrom, TryInto};
use std::time::{Duration, SystemTime, UNIX_EPOCH}; use std::time::{Duration, SystemTime, UNIX_EPOCH};
/// Added at the start of `.hg/dirstate` when the "v2" format is used. /// Added at the start of `.hg/dirstate` when the "v2" format is used.
/// This a redundant sanity check more than an actual "magic number" since /// This a redundant sanity check more than an actual "magic number" since
/// `.hg/requires` already governs which format should be used. /// `.hg/requires` already governs which format should be used.
pub const V2_FORMAT_MARKER: &[u8; 12] = b"dirstate-v2\n"; pub const V2_FORMAT_MARKER: &[u8; 12] = b"dirstate-v2\n";
/// Keep space for 256-bit hashes /// Keep space for 256-bit hashes
.unwrap() .unwrap()
.clone(); .clone();
let p2 = Node::try_from(&self.header.parent_2[..USED_NODE_ID_BYTES]) let p2 = Node::try_from(&self.header.parent_2[..USED_NODE_ID_BYTES])
.unwrap() .unwrap()
.clone(); .clone();
DirstateParents { p1, p2 } DirstateParents { p1, p2 }
} }
pub fn data_size(&self) -> usize {
// This `unwrap` could only panic on a 16-bit CPU
self.header.data_size.get().try_into().unwrap()
}
pub fn data_filename(&self) -> String { pub fn data_filename(&self) -> String {
String::from_utf8(format_bytes!(b"dirstate.{}.d", self.uuid)).unwrap() String::from_utf8(format_bytes!(b"dirstate.{}.d", self.uuid)).unwrap()
} }
} }
pub fn read_docket( pub fn read_docket(
on_disk: &[u8], on_disk: &[u8],
) -> Result<Docket<'_>, DirstateV2ParseError> { ) -> Result<Docket<'_>, DirstateV2ParseError> {

rust/hg-cpython/src/dirstate/dirstate_map.rs

let parents = parents.map(|p| dirstate_parents_to_pytuple(py, &p)); let parents = parents.map(|p| dirstate_parents_to_pytuple(py, &p));
Ok((map, parents).to_py_object(py).into_object()) Ok((map, parents).to_py_object(py).into_object())
} }
/// Returns a DirstateMap /// Returns a DirstateMap
@staticmethod @staticmethod
def new_v2( def new_v2(
on_disk: PyBytes, on_disk: PyBytes,
data_size: usize,
) -> PyResult<PyObject> { ) -> PyResult<PyObject> {
let dirstate_error = |e: DirstateError| { let dirstate_error = |e: DirstateError| {
PyErr::new::<exc::OSError, _>(py, format!("Dirstate error: {:?}", e)) PyErr::new::<exc::OSError, _>(py, format!("Dirstate error: {:?}", e))
}; };
let inner = OwningDirstateMap::new_v2(py, on_disk) let inner = OwningDirstateMap::new_v2(py, on_disk, data_size)
.map_err(dirstate_error)?; .map_err(dirstate_error)?;
let map = Self::create_instance(py, Box::new(inner))?; let map = Self::create_instance(py, Box::new(inner))?;
Ok(map.into_object()) Ok(map.into_object())
} }
def clear(&self) -> PyResult<PyObject> { def clear(&self) -> PyResult<PyObject> {
self.inner(py).borrow_mut().clear(); self.inner(py).borrow_mut().clear();
Ok(py.None()) Ok(py.None())

rust/hg-cpython/src/dirstate/owning.rs

let ptr: *mut () = ptr.cast(); let ptr: *mut () = ptr.cast();
Ok((Self { on_disk, ptr }, parents)) Ok((Self { on_disk, ptr }, parents))
} }
pub fn new_v2( pub fn new_v2(
py: Python, py: Python,
on_disk: PyBytes, on_disk: PyBytes,
data_size: usize,
) -> Result<Self, DirstateError> { ) -> Result<Self, DirstateError> {
let bytes: &'_ [u8] = on_disk.data(py); let bytes: &'_ [u8] = on_disk.data(py);
let map = DirstateMap::new_v2(bytes)?; let map = DirstateMap::new_v2(bytes, data_size)?;
// Like in `bytes` above, this `'_` lifetime parameter borrows from // Like in `bytes` above, this `'_` lifetime parameter borrows from
// the bytes buffer owned by `on_disk`. // the bytes buffer owned by `on_disk`.
let ptr: *mut DirstateMap<'_> = Box::into_raw(Box::new(map)); let ptr: *mut DirstateMap<'_> = Box::into_raw(Box::new(map));
// Erase the pointed type entirely in order to erase the lifetime. // Erase the pointed type entirely in order to erase the lifetime.
let ptr: *mut () = ptr.cast(); let ptr: *mut () = ptr.cast();

rust/rhg/src/commands/status.rs

} }
}; };
let repo = invocation.repo?; let repo = invocation.repo?;
let dirstate_data_mmap; let dirstate_data_mmap;
let (mut dmap, parents) = if repo.has_dirstate_v2() { let (mut dmap, parents) = if repo.has_dirstate_v2() {
let parents; let parents;
let dirstate_data; let dirstate_data;
let data_size;
if let Some(docket_data) = if let Some(docket_data) =
repo.hg_vfs().read("dirstate").io_not_found_as_none()? repo.hg_vfs().read("dirstate").io_not_found_as_none()?
{ {
let docket = on_disk::read_docket(&docket_data)?; let docket = on_disk::read_docket(&docket_data)?;
parents = Some(docket.parents()); parents = Some(docket.parents());
data_size = docket.data_size();
dirstate_data_mmap = repo dirstate_data_mmap = repo
.hg_vfs() .hg_vfs()
.mmap_open(docket.data_filename()) .mmap_open(docket.data_filename())
.io_not_found_as_none()?; .io_not_found_as_none()?;
dirstate_data = dirstate_data_mmap.as_deref().unwrap_or(b""); dirstate_data = dirstate_data_mmap.as_deref().unwrap_or(b"");
} else { } else {
parents = None; parents = None;
data_size = 0;
dirstate_data = b""; dirstate_data = b"";
} }
let dmap = DirstateMap::new_v2(dirstate_data)?; let dmap = DirstateMap::new_v2(dirstate_data, data_size)?;
(dmap, parents) (dmap, parents)
} else { } else {
dirstate_data_mmap = dirstate_data_mmap =
repo.hg_vfs().mmap_open("dirstate").io_not_found_as_none()?; repo.hg_vfs().mmap_open("dirstate").io_not_found_as_none()?;
let dirstate_data = dirstate_data_mmap.as_deref().unwrap_or(b""); let dirstate_data = dirstate_data_mmap.as_deref().unwrap_or(b"");
DirstateMap::new_v1(dirstate_data)? DirstateMap::new_v1(dirstate_data)?
}; };