Skip to content

Commit 340ee18

Browse files
committed
ValueDrain without RawLinks as potential fix of remove_entry_mult
Using RawLinks (to access links of other entries) is suspect for ValueDrain, particularly in `remove_entry_mult` where `remove_found` is used to remove the entire target entry (and adjust links accordingly). RawLinks was originally introduced to `ValueDrain` in 82d53db. Note that this introduces further duplication of code in `remove_extra_value_2` which isn't ideal.
1 parent ad54c55 commit 340ee18

1 file changed

Lines changed: 96 additions & 6 deletions

File tree

src/header/map.rs

Lines changed: 96 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,6 @@ pub struct ValueIterMut<'a, T> {
206206
/// An drain iterator of all values associated with a single header name.
207207
#[derive(Debug)]
208208
pub struct ValueDrain<'a, T> {
209-
raw_links: RawLinks<T>,
210209
extra_values: *mut Vec<ExtraValue<T>>,
211210
first: Option<T>,
212211
next: Option<usize>,
@@ -1192,11 +1191,9 @@ impl<T> HeaderMap<T> {
11921191
links = entry.links.take();
11931192
}
11941193

1195-
let raw_links = self.raw_links();
11961194
let extra_values = &mut self.extra_values as *mut _;
11971195

11981196
ValueDrain {
1199-
raw_links,
12001197
extra_values,
12011198
first: Some(old),
12021199
next: links.map(|l| l.next),
@@ -1703,6 +1700,101 @@ fn remove_extra_value<T>(mut raw_links: RawLinks<T>, extra_values: &mut Vec<Extr
17031700
extra
17041701
}
17051702

1703+
/// Removes the `ExtraValue` at the given index.
1704+
#[inline]
1705+
fn remove_extra_value_2<T>(extra_values: &mut Vec<ExtraValue<T>>, idx: usize) -> ExtraValue<T> {
1706+
let prev;
1707+
let next;
1708+
1709+
{
1710+
debug_assert!(extra_values.len() > idx);
1711+
let extra = &extra_values[idx];
1712+
prev = extra.prev;
1713+
next = extra.next;
1714+
}
1715+
1716+
// First unlink the extra value
1717+
match (prev, next) {
1718+
(Link::Entry(prev), Link::Entry(next)) => {
1719+
debug_assert_eq!(prev, next);
1720+
}
1721+
(Link::Entry(prev), Link::Extra(next)) => {
1722+
debug_assert!(extra_values.len() > next);
1723+
extra_values[next].prev = Link::Entry(prev);
1724+
}
1725+
(Link::Extra(prev), Link::Entry(next)) => {
1726+
debug_assert!(extra_values.len() > prev);
1727+
extra_values[prev].next = Link::Entry(next);
1728+
}
1729+
(Link::Extra(prev), Link::Extra(next)) => {
1730+
debug_assert!(extra_values.len() > next);
1731+
debug_assert!(extra_values.len() > prev);
1732+
1733+
extra_values[prev].next = Link::Extra(next);
1734+
extra_values[next].prev = Link::Extra(prev);
1735+
}
1736+
}
1737+
1738+
// Remove the extra value
1739+
let mut extra = extra_values.swap_remove(idx);
1740+
1741+
// This is the index of the value that was moved (possibly `extra`)
1742+
let old_idx = extra_values.len();
1743+
1744+
// Update the links
1745+
if extra.prev == Link::Extra(old_idx) {
1746+
extra.prev = Link::Extra(idx);
1747+
}
1748+
1749+
if extra.next == Link::Extra(old_idx) {
1750+
extra.next = Link::Extra(idx);
1751+
}
1752+
1753+
// Check if another entry was displaced. If it was, then the links
1754+
// need to be fixed.
1755+
if idx != old_idx {
1756+
let next;
1757+
let prev;
1758+
1759+
{
1760+
debug_assert!(extra_values.len() > idx);
1761+
let moved = &extra_values[idx];
1762+
next = moved.next;
1763+
prev = moved.prev;
1764+
}
1765+
1766+
// An entry was moved, we have to the links
1767+
match prev {
1768+
Link::Entry(_) => {
1769+
}
1770+
Link::Extra(extra_idx) => {
1771+
debug_assert!(extra_values.len() > extra_idx);
1772+
extra_values[extra_idx].next = Link::Extra(idx);
1773+
}
1774+
}
1775+
1776+
match next {
1777+
Link::Entry(_) => {
1778+
}
1779+
Link::Extra(extra_idx) => {
1780+
debug_assert!(extra_values.len() > extra_idx);
1781+
extra_values[extra_idx].prev = Link::Extra(idx);
1782+
}
1783+
}
1784+
}
1785+
1786+
debug_assert!({
1787+
for v in &*extra_values {
1788+
assert!(v.next != Link::Extra(old_idx));
1789+
assert!(v.prev != Link::Extra(old_idx));
1790+
}
1791+
1792+
true
1793+
});
1794+
1795+
extra
1796+
}
1797+
17061798
impl<'a, T> IntoIterator for &'a HeaderMap<T> {
17071799
type Item = (&'a HeaderName, &'a T);
17081800
type IntoIter = Iter<'a, T>;
@@ -2937,10 +3029,8 @@ impl<'a, T> OccupiedEntry<'a, T> {
29373029
/// returned.
29383030
pub fn remove_entry_mult(self) -> (HeaderName, ValueDrain<'a, T>) {
29393031
let entry = self.map.remove_found(self.probe, self.index);
2940-
let raw_links = self.map.raw_links();
29413032
let extra_values = &mut self.map.extra_values as *mut _;
29423033
let drain = ValueDrain {
2943-
raw_links,
29443034
extra_values,
29453035
first: Some(entry.value),
29463036
next: entry.links.map(|l| l.next),
@@ -3039,7 +3129,7 @@ impl<'a, T> Iterator for ValueDrain<'a, T> {
30393129
} else if let Some(next) = self.next {
30403130
// Remove the extra value
30413131
let extra = unsafe {
3042-
remove_extra_value(self.raw_links, &mut *self.extra_values, next)
3132+
remove_extra_value_2(&mut *self.extra_values, next)
30433133
};
30443134

30453135
match extra.next {

0 commit comments

Comments
 (0)