This is an archive of the discontinued Mercurial Phabricator instance.

Differential D12430

rust-dirstate: panic if the DirstateMap counters go below 0
ClosedPublic

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

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG2593873cda0f: rust-dirstate: panic if the DirstateMap counters go below 0
rHGfeace6e58eb7: rust-dirstate: panic if the DirstateMap counters go below 0
Summary

When modifying the API I hit some... interesting errors (trying to allocate
178GB of RAM, for example) because I failed to keep the counters correctly
updated.

This counter underflow is likely to happen when code is changed around
and can have up to eat-your-dirstate level of consequences, which is not nice.

The very small runtime cost of checking these counters should really not be an
issue and will help us uncover bugs when/if they do appear in the future.

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

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

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

Alphare added a commit: rHGfeace6e58eb7: rust-dirstate: panic if the DirstateMap counters go below 0.Apr 4 2022, 4:06 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGfeace6e58eb7: rust-dirstate: panic if the DirstateMap counters go below 0 (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 32770

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

if let Some((d, removed)) = recur( if let Some((d, removed)) = recur(
on_disk, on_disk,
unreachable_bytes, unreachable_bytes,
&mut node.children, &mut node.children,
rest, rest,
)? { )? {
dropped = d; dropped = d;
if dropped.had_entry { if dropped.had_entry {
node.descendants_with_entry_count -= 1; node.descendants_with_entry_count = node
.descendants_with_entry_count
.checked_sub(1)
.expect(
"descendants_with_entry_count should be >= 0",
);
} }
if dropped.was_tracked { if dropped.was_tracked {
node.tracked_descendants_count -= 1; node.tracked_descendants_count = node
.tracked_descendants_count
.checked_sub(1)
.expect(
"tracked_descendants_count should be >= 0",
);
} }
// Directory caches must be invalidated when removing a // Directory caches must be invalidated when removing a
// child node // child node
if removed { if removed {
if let NodeData::CachedDirectory { .. } = &node.data { if let NodeData::CachedDirectory { .. } = &node.data {
node.data = NodeData::None node.data = NodeData::None
} }
self.with_dmap_mut(|map| { self.with_dmap_mut(|map| {
if let Some((dropped, _removed)) = recur( if let Some((dropped, _removed)) = recur(
map.on_disk, map.on_disk,
&mut map.unreachable_bytes, &mut map.unreachable_bytes,
&mut map.root, &mut map.root,
filename, filename,
)? { )? {
if dropped.had_entry { if dropped.had_entry {
map.nodes_with_entry_count -= 1 map.nodes_with_entry_count = map
.nodes_with_entry_count
.checked_sub(1)
.expect("nodes_with_entry_count should be >= 0");
} }
if dropped.had_copy_source { if dropped.had_copy_source {
map.nodes_with_copy_source_count -= 1 map.nodes_with_copy_source_count = map
.nodes_with_copy_source_count
.checked_sub(1)
.expect("nodes_with_copy_source_count should be >= 0");
} }
} else { } else {
debug_assert!(!was_tracked); debug_assert!(!was_tracked);
} }
Ok(()) Ok(())
}) })
} }