From d205dc92de1d752a2913bc0377372726d75801f4 Mon Sep 17 00:00:00 2001 From: Will Lillis Date: Thu, 23 Jul 2026 02:55:12 -0400 Subject: [PATCH] fix(rust)!: require parser loggers to be `Send + 'static` `Parser::set_logger` boxes the logger callback into a type-erased C pointer, but `Parser` carries no lifetime parameter. This allows for two kinds of UB: - Use-after-free: a logger could capture a non-`'static` borrow, let the borrowed value drop, and then dangle the next time the parser logged. - Data race: a logger could capture a `!Send` value such as an `Rc`. The parser could then be moved to another thread and logged from there while the original thread still held a clone, racing on the reference count. As a fix, we just require that logger is `Send + 'static`. The alternative here is to attach a lifetime parameter to `Parser`, but this is highly breaking for what is mostly a debugging utility. As an escape hatch `set_logger_unchecked` is added to the public API as an `unsafe fn` to correctly communicate the risks and invariants that must be held. --- crates/cli/src/tests/parser_test.rs | 24 ++++++++++++++++-------- lib/binding_rust/lib.rs | 21 ++++++++++++++++++++- 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/crates/cli/src/tests/parser_test.rs b/crates/cli/src/tests/parser_test.rs index 32e6e5872..1278d4f47 100644 --- a/crates/cli/src/tests/parser_test.rs +++ b/crates/cli/src/tests/parser_test.rs @@ -66,9 +66,13 @@ fn test_parsing_with_logging() { parser.set_language(&get_language("rust")).unwrap(); let mut messages = Vec::new(); - parser.set_logger(Some(Box::new(|log_type, message| { - messages.push((log_type, message.to_string())); - }))); + // SAFETY: the logger borrows `messages` and is only invoked during the + // `parse` call below while `messages` is in scope. + unsafe { + parser.set_logger_unchecked(Some(Box::new(|log_type, message| { + messages.push((log_type, message.to_string())); + }))); + } parser .parse( @@ -1785,11 +1789,15 @@ fn test_parsing_with_scanner_logging() { .unwrap(); let mut found = false; - parser.set_logger(Some(Box::new(|log_type, message| { - if log_type == LogType::Lex && message == "Found a percent string" { - found = true; - } - }))); + // SAFETY: the logger borrows `found` and is only invoked during the `parse` + // call below, while `found` is in scope. + unsafe { + parser.set_logger_unchecked(Some(Box::new(|log_type, message| { + if log_type == LogType::Lex && message == "Found a percent string" { + found = true; + } + }))); + } let source_code = "x + %(sup (external) scanner?)"; diff --git a/lib/binding_rust/lib.rs b/lib/binding_rust/lib.rs index de2331752..60e0ac455 100644 --- a/lib/binding_rust/lib.rs +++ b/lib/binding_rust/lib.rs @@ -301,7 +301,10 @@ pub enum LogType { type FieldId = NonZeroU16; /// A callback that receives log messages during parsing. -type Logger<'a> = Box; +type Logger = Box; + +/// A callback that receives log messages during parsing, with relaxed constraints. +type UnsafeLogger<'a> = Box; /// A callback that receives the parse state during parsing. type ParseProgressCallback<'a> = &'a mut dyn FnMut(&ParseState) -> ControlFlow<()>; @@ -767,8 +770,24 @@ impl Parser { } /// Set the logging callback that the parser should use during parsing. + /// + /// To log through a callback that borrows non-`'static` data, see + /// [`set_logger_unchecked`](Parser::set_logger_unchecked). #[doc(alias = "ts_parser_set_logger")] pub fn set_logger(&mut self, logger: Option) { + // SAFETY: `Logger` is `Send + 'static` + unsafe { self.set_logger_unchecked(logger) }; + } + + /// Set the logging callback, allowing the callback to borrow non-`'static` + /// data. See [`set_logger`](Parser::set_logger). + /// + /// # Safety + /// + /// Any data borrowed by `logger` must remain valid until the logger is + /// removed. Equivalently, the parser must not be used to parse once the + /// borrowed data has gone out of scope. + pub unsafe fn set_logger_unchecked(&mut self, logger: Option>) { let prev_logger = unsafe { ffi::ts_parser_logger(self.0.as_ptr()) }; if !prev_logger.payload.is_null() { drop(unsafe { Box::from_raw(prev_logger.payload.cast::()) });