Skip to content

Commit

Permalink
Stop re-using NthIndexCache to avoid ABA problem
Browse files Browse the repository at this point in the history
I just noticed that the NthIndexCache is internally keyed on the pointer to the
selectors themselves which I think opens this up for an ABA problem, i.e. after
dropping a selector and allocating a new one at the same address but with
different content, the cache might result in incorrect matches.

Hence, I just avoided re-use completely which is certainly correct if
unnecessarily inefficient, but I fear proper re-use would need to be
user-visible.
  • Loading branch information
adamreichold committed Jun 26, 2023
1 parent ab207c9 commit 7fdac0a
Showing 1 changed file with 13 additions and 21 deletions.
34 changes: 13 additions & 21 deletions src/selector.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
//! CSS selectors.

use std::cell::RefCell;
use std::convert::TryFrom;
use std::fmt;

Expand All @@ -10,7 +9,6 @@ use html5ever::{LocalName, Namespace};
use selectors::{
matching,
parser::{self, ParseRelative, SelectorParseErrorKind},
NthIndexCache,
};

use crate::error::SelectorErrorKind;
Expand Down Expand Up @@ -46,25 +44,19 @@ impl Selector {
/// The optional `scope` argument is used to specify which element has `:scope` pseudo-class.
/// When it is `None`, `:scope` will match the root element.
pub fn matches_with_scope(&self, element: &ElementRef, scope: Option<ElementRef>) -> bool {
thread_local! {
static NTH_INDEX_CACHE: RefCell<NthIndexCache> = Default::default();
}

NTH_INDEX_CACHE.with(|nth_index_cache| {
let mut nth_index_cache = nth_index_cache.borrow_mut();
let mut context = matching::MatchingContext::new(
matching::MatchingMode::Normal,
None,
&mut nth_index_cache,
matching::QuirksMode::NoQuirks,
matching::NeedsSelectorFlags::No,
matching::IgnoreNthChildForInvalidation::No,
);
context.scope_element = scope.map(|x| selectors::Element::opaque(&x));
self.selectors
.iter()
.any(|s| matching::matches_selector(s, 0, None, element, &mut context))
})
let mut nth_index_cache = Default::default();
let mut context = matching::MatchingContext::new(
matching::MatchingMode::Normal,
None,
&mut nth_index_cache,
matching::QuirksMode::NoQuirks,
matching::NeedsSelectorFlags::No,
matching::IgnoreNthChildForInvalidation::No,
);
context.scope_element = scope.map(|x| selectors::Element::opaque(&x));
self.selectors
.iter()
.any(|s| matching::matches_selector(s, 0, None, element, &mut context))
}
}

Expand Down

0 comments on commit 7fdac0a

Please sign in to comment.