Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Mark query_captures function as unsafe
Browse files Browse the repository at this point in the history
It's easy to mistakenly use-after-free the cursor and captures iterator
here because of the transmute. Ideally this could be fixed upstream in
tree-sitter by introducing an API with lifetimes/types that reflect the
lifetimes of the underlying data.

Co-authored-by: Pascal Kuthe <[email protected]>
the-mikedavis and pascalkuthe committed Jan 12, 2024

Partially verified

This commit is signed with the committer’s verified signature.
the-mikedavis’s contribution has been verified via SSH key.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
1 parent ca4417c commit 35d707d
Showing 1 changed file with 20 additions and 14 deletions.
34 changes: 20 additions & 14 deletions helix-core/src/syntax.rs
Original file line number Diff line number Diff line change
@@ -942,7 +942,8 @@ thread_local! {

/// Creates an iterator over the captures in a query within the given range,
/// re-using a cursor from the pool if available.
fn query_captures<'a, 'tree>(
/// SAFETY: The `QueryCaptures` must be droped before the `QueryCursor` is dropped.
unsafe fn query_captures<'a, 'tree>(
query: &'a Query,
root: Node<'tree>,
source: RopeSlice<'a>,
@@ -957,10 +958,11 @@ fn query_captures<'a, 'tree>(
highlighter.cursors.pop().unwrap_or_else(QueryCursor::new)
});

// This is the unsafe line:
// The `captures` iterator borrows the `Tree` and the `QueryCursor`, which
// prevents them from being moved. But both of these values are really just
// pointers, so it's actually ok to move them.
let cursor_ref = unsafe { mem::transmute::<_, &'static mut QueryCursor>(&mut cursor) };
let cursor_ref = mem::transmute::<_, &'static mut QueryCursor>(&mut cursor);

// if reusing cursors & no range this resets to whole range
cursor_ref.set_byte_range(range.unwrap_or(0..usize::MAX));
@@ -1318,12 +1320,14 @@ impl Syntax {
.layers
.iter()
.filter_map(|(_, layer)| {
let (cursor, captures) = query_captures(
query_fn(&layer.config),
layer.tree().root_node(),
source,
range.clone(),
);
let (cursor, captures) = unsafe {
query_captures(
query_fn(&layer.config),
layer.tree().root_node(),
source,
range.clone(),
)
};
let mut captures = captures.peekable();

// If there aren't any captures for this layer, skip the layer.
@@ -1355,12 +1359,14 @@ impl Syntax {
.filter_map(|(_, layer)| {
// TODO: if range doesn't overlap layer range, skip it

let (cursor, captures) = query_captures(
&layer.config.query,
layer.tree().root_node(),
source,
range.clone(),
);
let (cursor, captures) = unsafe {
query_captures(
&layer.config.query,
layer.tree().root_node(),
source,
range.clone(),
)
};
let mut captures = captures.peekable();

// If there are no captures, skip the layer

0 comments on commit 35d707d

Please sign in to comment.