From ac579be788001916024083b3066a314a138d3b31 Mon Sep 17 00:00:00 2001 From: Amaan Qureshi Date: Sat, 19 Aug 2023 18:59:11 -0400 Subject: [PATCH 1/3] fix(safety): mark functions that potentially deref a raw pointer as unsafe --- cli/src/tests/highlight_test.rs | 96 ++++++++++++++++++--------------- cli/src/tests/tags_test.rs | 50 +++++++++-------- highlight/src/c_lib.rs | 55 +++++++++++++------ tags/src/c_lib.rs | 37 ++++++++++--- 4 files changed, 150 insertions(+), 88 deletions(-) diff --git a/cli/src/tests/highlight_test.rs b/cli/src/tests/highlight_test.rs index 5cdd0f1d..c788c177 100644 --- a/cli/src/tests/highlight_test.rs +++ b/cli/src/tests/highlight_test.rs @@ -496,11 +496,13 @@ fn test_highlighting_via_c_api() { .iter() .map(|h| h.as_bytes().as_ptr() as *const c_char) .collect::>(); - let highlighter = c::ts_highlighter_new( - &highlight_names[0] as *const *const c_char, - &highlight_attrs[0] as *const *const c_char, - highlights.len() as u32, - ); + let highlighter = unsafe { + c::ts_highlighter_new( + &highlight_names[0] as *const *const c_char, + &highlight_attrs[0] as *const *const c_char, + highlights.len() as u32, + ) + }; let source_code = c_string(""); @@ -512,20 +514,22 @@ fn test_highlighting_via_c_api() { let highlights_query = fs::read_to_string(queries.join("highlights.scm")).unwrap(); let injections_query = fs::read_to_string(queries.join("injections.scm")).unwrap(); let locals_query = fs::read_to_string(queries.join("locals.scm")).unwrap(); - c::ts_highlighter_add_language( - highlighter, - lang_name.as_ptr(), - js_scope.as_ptr(), - js_injection_regex.as_ptr(), - language, - highlights_query.as_ptr() as *const c_char, - injections_query.as_ptr() as *const c_char, - locals_query.as_ptr() as *const c_char, - highlights_query.len() as u32, - injections_query.len() as u32, - locals_query.len() as u32, - false, - ); + unsafe { + c::ts_highlighter_add_language( + highlighter, + lang_name.as_ptr(), + js_scope.as_ptr(), + js_injection_regex.as_ptr(), + language, + highlights_query.as_ptr() as *const c_char, + injections_query.as_ptr() as *const c_char, + locals_query.as_ptr() as *const c_char, + highlights_query.len() as u32, + injections_query.len() as u32, + locals_query.len() as u32, + false, + ); + } let html_scope = c_string("text.html.basic"); let html_injection_regex = c_string("^html"); @@ -534,31 +538,35 @@ fn test_highlighting_via_c_api() { let queries = get_language_queries_path("html"); let highlights_query = fs::read_to_string(queries.join("highlights.scm")).unwrap(); let injections_query = fs::read_to_string(queries.join("injections.scm")).unwrap(); - c::ts_highlighter_add_language( - highlighter, - lang_name.as_ptr(), - html_scope.as_ptr(), - html_injection_regex.as_ptr(), - language, - highlights_query.as_ptr() as *const c_char, - injections_query.as_ptr() as *const c_char, - ptr::null(), - highlights_query.len() as u32, - injections_query.len() as u32, - 0, - false, - ); + unsafe { + c::ts_highlighter_add_language( + highlighter, + lang_name.as_ptr(), + html_scope.as_ptr(), + html_injection_regex.as_ptr(), + language, + highlights_query.as_ptr() as *const c_char, + injections_query.as_ptr() as *const c_char, + ptr::null(), + highlights_query.len() as u32, + injections_query.len() as u32, + 0, + false, + ); + } let buffer = c::ts_highlight_buffer_new(); - c::ts_highlighter_highlight( - highlighter, - html_scope.as_ptr(), - source_code.as_ptr(), - source_code.as_bytes().len() as u32, - buffer, - ptr::null_mut(), - ); + unsafe { + c::ts_highlighter_highlight( + highlighter, + html_scope.as_ptr(), + source_code.as_ptr(), + source_code.as_bytes().len() as u32, + buffer, + ptr::null_mut(), + ); + } let output_bytes = c::ts_highlight_buffer_content(buffer); let output_line_offsets = c::ts_highlight_buffer_line_offsets(buffer); @@ -589,8 +597,10 @@ fn test_highlighting_via_c_api() { ] ); - c::ts_highlighter_delete(highlighter); - c::ts_highlight_buffer_delete(buffer); + unsafe { + c::ts_highlighter_delete(highlighter); + c::ts_highlight_buffer_delete(buffer); + } } #[test] diff --git a/cli/src/tests/tags_test.rs b/cli/src/tests/tags_test.rs index 07e5d1de..20392749 100644 --- a/cli/src/tests/tags_test.rs +++ b/cli/src/tests/tags_test.rs @@ -9,7 +9,7 @@ use std::{ use tree_sitter::Point; use tree_sitter_tags::{c_lib as c, Error, TagsConfiguration, TagsContext}; -const PYTHON_TAG_QUERY: &'static str = r#" +const PYTHON_TAG_QUERY: &str = r#" ( (function_definition name: (identifier) @name @@ -39,7 +39,7 @@ const PYTHON_TAG_QUERY: &'static str = r#" attribute: (identifier) @name)) @reference.call "#; -const JS_TAG_QUERY: &'static str = r#" +const JS_TAG_QUERY: &str = r#" ( (comment)* @doc . (class_declaration @@ -68,7 +68,7 @@ const JS_TAG_QUERY: &'static str = r#" function: (identifier) @name) @reference.call "#; -const RUBY_TAG_QUERY: &'static str = r#" +const RUBY_TAG_QUERY: &str = r#" (method name: (_) @name) @definition.method @@ -359,25 +359,29 @@ fn test_tags_via_c_api() { ); let c_scope_name = CString::new(scope_name).unwrap(); - let result = c::ts_tagger_add_language( - tagger, - c_scope_name.as_ptr(), - language, - JS_TAG_QUERY.as_ptr(), - ptr::null(), - JS_TAG_QUERY.len() as u32, - 0, - ); + let result = unsafe { + c::ts_tagger_add_language( + tagger, + c_scope_name.as_ptr(), + language, + JS_TAG_QUERY.as_ptr(), + ptr::null(), + JS_TAG_QUERY.len() as u32, + 0, + ) + }; assert_eq!(result, c::TSTagsError::Ok); - let result = c::ts_tagger_tag( - tagger, - c_scope_name.as_ptr(), - source_code.as_ptr(), - source_code.len() as u32, - buffer, - ptr::null(), - ); + let result = unsafe { + c::ts_tagger_tag( + tagger, + c_scope_name.as_ptr(), + source_code.as_ptr(), + source_code.len() as u32, + buffer, + ptr::null(), + ) + }; assert_eq!(result, c::TSTagsError::Ok); let tags = unsafe { slice::from_raw_parts( @@ -419,8 +423,10 @@ fn test_tags_via_c_api() { ] ); - c::ts_tags_buffer_delete(buffer); - c::ts_tagger_delete(tagger); + unsafe { + c::ts_tags_buffer_delete(buffer); + c::ts_tagger_delete(tagger); + } }); } diff --git a/highlight/src/c_lib.rs b/highlight/src/c_lib.rs index 33197088..78cdd8c2 100644 --- a/highlight/src/c_lib.rs +++ b/highlight/src/c_lib.rs @@ -32,8 +32,14 @@ pub enum ErrorCode { InvalidLanguageName, } +/// Create a new [`TSHighlighter`] instance. +/// +/// # Safety +/// +/// The caller must ensure that the `highlight_names` and `attribute_strings` arrays are valid for +/// the lifetime of the returned [`TSHighlighter`] instance, and are non-null. #[no_mangle] -pub extern "C" fn ts_highlighter_new( +pub unsafe extern "C" fn ts_highlighter_new( highlight_names: *const *const c_char, attribute_strings: *const *const c_char, highlight_count: u32, @@ -43,11 +49,11 @@ pub extern "C" fn ts_highlighter_new( let attribute_strings = unsafe { slice::from_raw_parts(attribute_strings, highlight_count as usize) }; let highlight_names = highlight_names - .into_iter() + .iter() .map(|s| unsafe { CStr::from_ptr(*s).to_string_lossy().to_string() }) .collect::>(); let attribute_strings = attribute_strings - .into_iter() + .iter() .map(|s| unsafe { CStr::from_ptr(*s).to_bytes() }) .collect(); let carriage_return_index = highlight_names.iter().position(|s| s == "carriage-return"); @@ -59,8 +65,14 @@ pub extern "C" fn ts_highlighter_new( })) } +/// Add a language to a [`TSHighlighter`] instance. +/// +/// # Safety +/// +/// The caller must ensure that any `*const c_char` parameters are valid for the lifetime of +/// the [`TSHighlighter`] instance, and are non-null. #[no_mangle] -pub extern "C" fn ts_highlighter_add_language( +pub unsafe extern "C" fn ts_highlighter_add_language( this: *mut TSHighlighter, language_name: *const c_char, scope_name: *const c_char, @@ -125,7 +137,7 @@ pub extern "C" fn ts_highlighter_add_language( apply_all_captures, ) .or(Err(ErrorCode::InvalidQuery))?; - config.configure(&this.highlight_names.as_slice()); + config.configure(this.highlight_names.as_slice()); this.languages.insert(scope_name, (injection_regex, config)); Ok(()) @@ -145,13 +157,23 @@ pub extern "C" fn ts_highlight_buffer_new() -> *mut TSHighlightBuffer { })) } +/// Deleteis a [`TSHighlighter`] instance. +/// +/// # Safety +/// +/// `this` must be non-null. #[no_mangle] -pub extern "C" fn ts_highlighter_delete(this: *mut TSHighlighter) { +pub unsafe extern "C" fn ts_highlighter_delete(this: *mut TSHighlighter) { drop(unsafe { Box::from_raw(this) }) } +/// Deleteis a [`TSHighlightBuffer`] instance. +/// +/// # Safety +/// +/// `this` must be non-null. #[no_mangle] -pub extern "C" fn ts_highlight_buffer_delete(this: *mut TSHighlightBuffer) { +pub unsafe extern "C" fn ts_highlight_buffer_delete(this: *mut TSHighlightBuffer) { drop(unsafe { Box::from_raw(this) }) } @@ -179,8 +201,14 @@ pub extern "C" fn ts_highlight_buffer_line_count(this: *const TSHighlightBuffer) this.renderer.line_offsets.len() as u32 } +/// Highlight a string of source code. +/// +/// # Safety +/// +/// The caller must ensure that `scope_name`, `source_code`, and `cancellation_flag` are valid for +/// the lifetime of the [`TSHighlighter`] instance, and are non-null. #[no_mangle] -pub extern "C" fn ts_highlighter_highlight( +pub unsafe extern "C" fn ts_highlighter_highlight( this: *const TSHighlighter, scope_name: *const c_char, source_code: *const c_char, @@ -238,15 +266,8 @@ impl TSHighlighter { .renderer .render(highlights, source_code, &|s| self.attribute_strings[s.0]); match result { - Err(Error::Cancelled) => { - return ErrorCode::Timeout; - } - Err(Error::InvalidLanguage) => { - return ErrorCode::InvalidLanguage; - } - Err(Error::Unknown) => { - return ErrorCode::Timeout; - } + Err(Error::Cancelled) | Err(Error::Unknown) => ErrorCode::Timeout, + Err(Error::InvalidLanguage) => ErrorCode::InvalidLanguage, Ok(()) => ErrorCode::Ok, } } else { diff --git a/tags/src/c_lib.rs b/tags/src/c_lib.rs index c8f39d2c..0952d851 100644 --- a/tags/src/c_lib.rs +++ b/tags/src/c_lib.rs @@ -66,13 +66,23 @@ pub extern "C" fn ts_tagger_new() -> *mut TSTagger { })) } +/// Delete a TSTagger. +/// +/// # Safety +/// +/// `this` must be non-null #[no_mangle] -pub extern "C" fn ts_tagger_delete(this: *mut TSTagger) { +pub unsafe extern "C" fn ts_tagger_delete(this: *mut TSTagger) { drop(unsafe { Box::from_raw(this) }) } +/// Add a language to a TSTagger. +/// +/// # Safety +/// +/// `this` must be non-null #[no_mangle] -pub extern "C" fn ts_tagger_add_language( +pub unsafe extern "C" fn ts_tagger_add_language( this: *mut TSTagger, scope_name: *const c_char, language: Language, @@ -84,7 +94,7 @@ pub extern "C" fn ts_tagger_add_language( let tagger = unwrap_mut_ptr(this); let scope_name = unsafe { unwrap(CStr::from_ptr(scope_name).to_str()) }; let tags_query = unsafe { slice::from_raw_parts(tags_query, tags_query_len as usize) }; - let locals_query = if locals_query != std::ptr::null() { + let locals_query = if !locals_query.is_null() { unsafe { slice::from_raw_parts(locals_query, locals_query_len as usize) } } else { &[] @@ -111,8 +121,13 @@ pub extern "C" fn ts_tagger_add_language( } } +/// Tag some source code. +/// +/// # Safety +/// +/// `this` must be non-null #[no_mangle] -pub extern "C" fn ts_tagger_tag( +pub unsafe extern "C" fn ts_tagger_tag( this: *mut TSTagger, scope_name: *const c_char, source_code: *const u8, @@ -201,8 +216,13 @@ pub extern "C" fn ts_tags_buffer_new() -> *mut TSTagsBuffer { })) } +/// Delete a TSTagsBuffer. +/// +/// # Safety +/// +/// `this` must be non-null #[no_mangle] -pub extern "C" fn ts_tags_buffer_delete(this: *mut TSTagsBuffer) { +pub unsafe extern "C" fn ts_tags_buffer_delete(this: *mut TSTagsBuffer) { drop(unsafe { Box::from_raw(this) }) } @@ -236,8 +256,13 @@ pub extern "C" fn ts_tags_buffer_found_parse_error(this: *const TSTagsBuffer) -> buffer.errors_present } +/// Get the syntax kinds for a given scope name. +/// +/// # Safety +/// +/// `this` must be non-null #[no_mangle] -pub extern "C" fn ts_tagger_syntax_kinds_for_scope_name( +pub unsafe extern "C" fn ts_tagger_syntax_kinds_for_scope_name( this: *mut TSTagger, scope_name: *const c_char, len: *mut u32, From ffae7d611563f0a7e6fcfafbcb34e14f0c722a9d Mon Sep 17 00:00:00 2001 From: Andrew Hlynskyi Date: Sun, 20 Aug 2023 01:48:17 +0300 Subject: [PATCH 2/3] fix: mark helper Rust funcs that receive raw pointers as unsafe --- cli/src/tests/highlight_test.rs | 8 ++--- highlight/src/c_lib.rs | 62 +++++++++++++++------------------ tags/src/c_lib.rs | 34 +++++++++--------- 3 files changed, 50 insertions(+), 54 deletions(-) diff --git a/cli/src/tests/highlight_test.rs b/cli/src/tests/highlight_test.rs index c788c177..c4ca0b49 100644 --- a/cli/src/tests/highlight_test.rs +++ b/cli/src/tests/highlight_test.rs @@ -568,10 +568,10 @@ fn test_highlighting_via_c_api() { ); } - let output_bytes = c::ts_highlight_buffer_content(buffer); - let output_line_offsets = c::ts_highlight_buffer_line_offsets(buffer); - let output_len = c::ts_highlight_buffer_len(buffer); - let output_line_count = c::ts_highlight_buffer_line_count(buffer); + let output_bytes = unsafe { c::ts_highlight_buffer_content(buffer) }; + let output_line_offsets = unsafe { c::ts_highlight_buffer_line_offsets(buffer) }; + let output_len = unsafe { c::ts_highlight_buffer_len(buffer) }; + let output_line_count = unsafe { c::ts_highlight_buffer_line_count(buffer) }; let output_bytes = unsafe { slice::from_raw_parts(output_bytes, output_len as usize) }; let output_line_offsets = diff --git a/highlight/src/c_lib.rs b/highlight/src/c_lib.rs index 78cdd8c2..2fc6934e 100644 --- a/highlight/src/c_lib.rs +++ b/highlight/src/c_lib.rs @@ -44,17 +44,15 @@ pub unsafe extern "C" fn ts_highlighter_new( attribute_strings: *const *const c_char, highlight_count: u32, ) -> *mut TSHighlighter { - let highlight_names = - unsafe { slice::from_raw_parts(highlight_names, highlight_count as usize) }; - let attribute_strings = - unsafe { slice::from_raw_parts(attribute_strings, highlight_count as usize) }; + let highlight_names = slice::from_raw_parts(highlight_names, highlight_count as usize); + let attribute_strings = slice::from_raw_parts(attribute_strings, highlight_count as usize); let highlight_names = highlight_names - .iter() - .map(|s| unsafe { CStr::from_ptr(*s).to_string_lossy().to_string() }) + .into_iter() + .map(|s| CStr::from_ptr(*s).to_string_lossy().to_string()) .collect::>(); let attribute_strings = attribute_strings - .iter() - .map(|s| unsafe { CStr::from_ptr(*s).to_bytes() }) + .into_iter() + .map(|s| CStr::from_ptr(*s).to_bytes()) .collect(); let carriage_return_index = highlight_names.iter().position(|s| s == "carriage-return"); Box::into_raw(Box::new(TSHighlighter { @@ -88,7 +86,7 @@ pub unsafe extern "C" fn ts_highlighter_add_language( ) -> ErrorCode { let f = move || { let this = unwrap_mut_ptr(this); - let scope_name = unsafe { CStr::from_ptr(scope_name) }; + let scope_name = CStr::from_ptr(scope_name); let scope_name = scope_name .to_str() .or(Err(ErrorCode::InvalidUtf8))? @@ -96,29 +94,26 @@ pub unsafe extern "C" fn ts_highlighter_add_language( let injection_regex = if injection_regex.is_null() { None } else { - let pattern = unsafe { CStr::from_ptr(injection_regex) }; + let pattern = CStr::from_ptr(injection_regex); let pattern = pattern.to_str().or(Err(ErrorCode::InvalidUtf8))?; Some(Regex::new(pattern).or(Err(ErrorCode::InvalidRegex))?) }; - let highlight_query = unsafe { - slice::from_raw_parts(highlight_query as *const u8, highlight_query_len as usize) - }; + let highlight_query = + slice::from_raw_parts(highlight_query as *const u8, highlight_query_len as usize); + let highlight_query = str::from_utf8(highlight_query).or(Err(ErrorCode::InvalidUtf8))?; let injection_query = if injection_query_len > 0 { - let query = unsafe { - slice::from_raw_parts(injection_query as *const u8, injection_query_len as usize) - }; + let query = + slice::from_raw_parts(injection_query as *const u8, injection_query_len as usize); str::from_utf8(query).or(Err(ErrorCode::InvalidUtf8))? } else { "" }; let locals_query = if locals_query_len > 0 { - let query = unsafe { - slice::from_raw_parts(locals_query as *const u8, locals_query_len as usize) - }; + let query = slice::from_raw_parts(locals_query as *const u8, locals_query_len as usize); str::from_utf8(query).or(Err(ErrorCode::InvalidUtf8))? } else { "" @@ -164,7 +159,7 @@ pub extern "C" fn ts_highlight_buffer_new() -> *mut TSHighlightBuffer { /// `this` must be non-null. #[no_mangle] pub unsafe extern "C" fn ts_highlighter_delete(this: *mut TSHighlighter) { - drop(unsafe { Box::from_raw(this) }) + drop(Box::from_raw(this)) } /// Deleteis a [`TSHighlightBuffer`] instance. @@ -174,29 +169,31 @@ pub unsafe extern "C" fn ts_highlighter_delete(this: *mut TSHighlighter) { /// `this` must be non-null. #[no_mangle] pub unsafe extern "C" fn ts_highlight_buffer_delete(this: *mut TSHighlightBuffer) { - drop(unsafe { Box::from_raw(this) }) + drop(Box::from_raw(this)) } #[no_mangle] -pub extern "C" fn ts_highlight_buffer_content(this: *const TSHighlightBuffer) -> *const u8 { +pub unsafe extern "C" fn ts_highlight_buffer_content(this: *const TSHighlightBuffer) -> *const u8 { let this = unwrap_ptr(this); this.renderer.html.as_slice().as_ptr() } #[no_mangle] -pub extern "C" fn ts_highlight_buffer_line_offsets(this: *const TSHighlightBuffer) -> *const u32 { +pub unsafe extern "C" fn ts_highlight_buffer_line_offsets( + this: *const TSHighlightBuffer, +) -> *const u32 { let this = unwrap_ptr(this); this.renderer.line_offsets.as_slice().as_ptr() } #[no_mangle] -pub extern "C" fn ts_highlight_buffer_len(this: *const TSHighlightBuffer) -> u32 { +pub unsafe extern "C" fn ts_highlight_buffer_len(this: *const TSHighlightBuffer) -> u32 { let this = unwrap_ptr(this); this.renderer.html.len() as u32 } #[no_mangle] -pub extern "C" fn ts_highlight_buffer_line_count(this: *const TSHighlightBuffer) -> u32 { +pub unsafe extern "C" fn ts_highlight_buffer_line_count(this: *const TSHighlightBuffer) -> u32 { let this = unwrap_ptr(this); this.renderer.line_offsets.len() as u32 } @@ -218,10 +215,9 @@ pub unsafe extern "C" fn ts_highlighter_highlight( ) -> ErrorCode { let this = unwrap_ptr(this); let output = unwrap_mut_ptr(output); - let scope_name = unwrap(unsafe { CStr::from_ptr(scope_name).to_str() }); - let source_code = - unsafe { slice::from_raw_parts(source_code as *const u8, source_code_len as usize) }; - let cancellation_flag = unsafe { cancellation_flag.as_ref() }; + let scope_name = unwrap(CStr::from_ptr(scope_name).to_str()); + let source_code = slice::from_raw_parts(source_code as *const u8, source_code_len as usize); + let cancellation_flag = cancellation_flag.as_ref(); this.highlight(source_code, scope_name, output, cancellation_flag) } @@ -276,15 +272,15 @@ impl TSHighlighter { } } -fn unwrap_ptr<'a, T>(result: *const T) -> &'a T { - unsafe { result.as_ref() }.unwrap_or_else(|| { +unsafe fn unwrap_ptr<'a, T>(result: *const T) -> &'a T { + result.as_ref().unwrap_or_else(|| { eprintln!("{}:{} - pointer must not be null", file!(), line!()); abort(); }) } -fn unwrap_mut_ptr<'a, T>(result: *mut T) -> &'a mut T { - unsafe { result.as_mut() }.unwrap_or_else(|| { +unsafe fn unwrap_mut_ptr<'a, T>(result: *mut T) -> &'a mut T { + result.as_mut().unwrap_or_else(|| { eprintln!("{}:{} - pointer must not be null", file!(), line!()); abort(); }) diff --git a/tags/src/c_lib.rs b/tags/src/c_lib.rs index 0952d851..df21f181 100644 --- a/tags/src/c_lib.rs +++ b/tags/src/c_lib.rs @@ -73,7 +73,7 @@ pub extern "C" fn ts_tagger_new() -> *mut TSTagger { /// `this` must be non-null #[no_mangle] pub unsafe extern "C" fn ts_tagger_delete(this: *mut TSTagger) { - drop(unsafe { Box::from_raw(this) }) + drop(Box::from_raw(this)) } /// Add a language to a TSTagger. @@ -92,10 +92,10 @@ pub unsafe extern "C" fn ts_tagger_add_language( locals_query_len: u32, ) -> TSTagsError { let tagger = unwrap_mut_ptr(this); - let scope_name = unsafe { unwrap(CStr::from_ptr(scope_name).to_str()) }; - let tags_query = unsafe { slice::from_raw_parts(tags_query, tags_query_len as usize) }; - let locals_query = if !locals_query.is_null() { - unsafe { slice::from_raw_parts(locals_query, locals_query_len as usize) } + let scope_name = unwrap(CStr::from_ptr(scope_name).to_str()); + let tags_query = slice::from_raw_parts(tags_query, tags_query_len as usize); + let locals_query = if locals_query != std::ptr::null() { + slice::from_raw_parts(locals_query, locals_query_len as usize) } else { &[] }; @@ -137,14 +137,14 @@ pub unsafe extern "C" fn ts_tagger_tag( ) -> TSTagsError { let tagger = unwrap_mut_ptr(this); let buffer = unwrap_mut_ptr(output); - let scope_name = unsafe { unwrap(CStr::from_ptr(scope_name).to_str()) }; + let scope_name = unwrap(CStr::from_ptr(scope_name).to_str()); if let Some(config) = tagger.languages.get(scope_name) { shrink_and_clear(&mut buffer.tags, BUFFER_TAGS_RESERVE_CAPACITY); shrink_and_clear(&mut buffer.docs, BUFFER_DOCS_RESERVE_CAPACITY); - let source_code = unsafe { slice::from_raw_parts(source_code, source_code_len as usize) }; - let cancellation_flag = unsafe { cancellation_flag.as_ref() }; + let source_code = slice::from_raw_parts(source_code, source_code_len as usize); + let cancellation_flag = cancellation_flag.as_ref(); let tags = match buffer .context @@ -223,35 +223,35 @@ pub extern "C" fn ts_tags_buffer_new() -> *mut TSTagsBuffer { /// `this` must be non-null #[no_mangle] pub unsafe extern "C" fn ts_tags_buffer_delete(this: *mut TSTagsBuffer) { - drop(unsafe { Box::from_raw(this) }) + drop(Box::from_raw(this)) } #[no_mangle] -pub extern "C" fn ts_tags_buffer_tags(this: *const TSTagsBuffer) -> *const TSTag { +pub unsafe extern "C" fn ts_tags_buffer_tags(this: *const TSTagsBuffer) -> *const TSTag { let buffer = unwrap_ptr(this); buffer.tags.as_ptr() } #[no_mangle] -pub extern "C" fn ts_tags_buffer_tags_len(this: *const TSTagsBuffer) -> u32 { +pub unsafe extern "C" fn ts_tags_buffer_tags_len(this: *const TSTagsBuffer) -> u32 { let buffer = unwrap_ptr(this); buffer.tags.len() as u32 } #[no_mangle] -pub extern "C" fn ts_tags_buffer_docs(this: *const TSTagsBuffer) -> *const c_char { +pub unsafe extern "C" fn ts_tags_buffer_docs(this: *const TSTagsBuffer) -> *const c_char { let buffer = unwrap_ptr(this); buffer.docs.as_ptr() as *const c_char } #[no_mangle] -pub extern "C" fn ts_tags_buffer_docs_len(this: *const TSTagsBuffer) -> u32 { +pub unsafe extern "C" fn ts_tags_buffer_docs_len(this: *const TSTagsBuffer) -> u32 { let buffer = unwrap_ptr(this); buffer.docs.len() as u32 } #[no_mangle] -pub extern "C" fn ts_tags_buffer_found_parse_error(this: *const TSTagsBuffer) -> bool { +pub unsafe extern "C" fn ts_tags_buffer_found_parse_error(this: *const TSTagsBuffer) -> bool { let buffer = unwrap_ptr(this); buffer.errors_present } @@ -268,7 +268,7 @@ pub unsafe extern "C" fn ts_tagger_syntax_kinds_for_scope_name( len: *mut u32, ) -> *const *const c_char { let tagger = unwrap_mut_ptr(this); - let scope_name = unsafe { unwrap(CStr::from_ptr(scope_name).to_str()) }; + let scope_name = unwrap(CStr::from_ptr(scope_name).to_str()); let len = unwrap_mut_ptr(len); *len = 0; @@ -279,14 +279,14 @@ pub unsafe extern "C" fn ts_tagger_syntax_kinds_for_scope_name( std::ptr::null() } -fn unwrap_ptr<'a, T>(result: *const T) -> &'a T { +unsafe fn unwrap_ptr<'a, T>(result: *const T) -> &'a T { unsafe { result.as_ref() }.unwrap_or_else(|| { eprintln!("{}:{} - pointer must not be null", file!(), line!()); abort(); }) } -fn unwrap_mut_ptr<'a, T>(result: *mut T) -> &'a mut T { +unsafe fn unwrap_mut_ptr<'a, T>(result: *mut T) -> &'a mut T { unsafe { result.as_mut() }.unwrap_or_else(|| { eprintln!("{}:{} - pointer must not be null", file!(), line!()); abort(); From c332066666b1bced7575aacd8f469d13b1e63437 Mon Sep 17 00:00:00 2001 From: Amaan Qureshi Date: Sat, 19 Aug 2023 19:42:18 -0400 Subject: [PATCH 3/3] fix(safety): improve docs for unsafe C functions --- highlight/src/c_lib.rs | 65 +++++++++++++++++++++++++++++----- tags/src/c_lib.rs | 79 +++++++++++++++++++++++++++++++++++++----- 2 files changed, 126 insertions(+), 18 deletions(-) diff --git a/highlight/src/c_lib.rs b/highlight/src/c_lib.rs index 2fc6934e..1ab4903a 100644 --- a/highlight/src/c_lib.rs +++ b/highlight/src/c_lib.rs @@ -47,11 +47,11 @@ pub unsafe extern "C" fn ts_highlighter_new( let highlight_names = slice::from_raw_parts(highlight_names, highlight_count as usize); let attribute_strings = slice::from_raw_parts(attribute_strings, highlight_count as usize); let highlight_names = highlight_names - .into_iter() + .iter() .map(|s| CStr::from_ptr(*s).to_string_lossy().to_string()) .collect::>(); let attribute_strings = attribute_strings - .into_iter() + .iter() .map(|s| CStr::from_ptr(*s).to_bytes()) .collect(); let carriage_return_index = highlight_names.iter().position(|s| s == "carriage-return"); @@ -65,9 +65,14 @@ pub unsafe extern "C" fn ts_highlighter_new( /// Add a language to a [`TSHighlighter`] instance. /// +/// Returns an [`ErrorCode`] indicating whether the language was added successfully or not. +/// /// # Safety /// -/// The caller must ensure that any `*const c_char` parameters are valid for the lifetime of +/// `this` must be non-null and must be a valid pointer to a [`TSHighlighter`] instance +/// created by [`ts_highlighter_new`]. +/// +/// The caller must ensure that any `*const c_char` (C-style string) parameters are valid for the lifetime of /// the [`TSHighlighter`] instance, and are non-null. #[no_mangle] pub unsafe extern "C" fn ts_highlighter_add_language( @@ -119,7 +124,7 @@ pub unsafe extern "C" fn ts_highlighter_add_language( "" }; - let lang = unsafe { CStr::from_ptr(language_name) } + let lang = CStr::from_ptr(language_name) .to_str() .or(Err(ErrorCode::InvalidLanguageName))?; @@ -152,32 +157,60 @@ pub extern "C" fn ts_highlight_buffer_new() -> *mut TSHighlightBuffer { })) } -/// Deleteis a [`TSHighlighter`] instance. +/// Deletes a [`TSHighlighter`] instance. /// /// # Safety /// -/// `this` must be non-null. +/// `this` must be non-null and must be a valid pointer to a [`TSHighlighter`] instance +/// created by [`ts_highlighter_new`]. +/// +/// It cannot be used after this function is called. #[no_mangle] pub unsafe extern "C" fn ts_highlighter_delete(this: *mut TSHighlighter) { drop(Box::from_raw(this)) } -/// Deleteis a [`TSHighlightBuffer`] instance. +/// Deletes a [`TSHighlightBuffer`] instance. /// /// # Safety /// -/// `this` must be non-null. +/// `this` must be non-null and must be a valid pointer to a [`TSHighlightBuffer`] instance +/// created by [`ts_highlight_buffer_new`] +/// +/// It cannot be used after this function is called. #[no_mangle] pub unsafe extern "C" fn ts_highlight_buffer_delete(this: *mut TSHighlightBuffer) { drop(Box::from_raw(this)) } +/// Get the HTML content of a [`TSHighlightBuffer`] instance as a raw pointer. +/// +/// # Safety +/// +/// `this` must be non-null and must be a valid pointer to a [`TSHighlightBuffer`] instance +/// created by [`ts_highlight_buffer_new`]. +/// +/// The returned pointer, a C-style string, must not outlive the [`TSHighlightBuffer`] instance, else the +/// data will point to garbage. +/// +/// To get the length of the HTML content, use [`ts_highlight_buffer_len`]. #[no_mangle] pub unsafe extern "C" fn ts_highlight_buffer_content(this: *const TSHighlightBuffer) -> *const u8 { let this = unwrap_ptr(this); this.renderer.html.as_slice().as_ptr() } +/// Get the line offsets of a [`TSHighlightBuffer`] instance as a C-style array. +/// +/// # Safety +/// +/// `this` must be non-null and must be a valid pointer to a [`TSHighlightBuffer`] instance +/// created by [`ts_highlight_buffer_new`]. +/// +/// The returned pointer, a C-style array of [`u32`]s, must not outlive the [`TSHighlightBuffer`] instance, else the +/// data will point to garbage. +/// +/// To get the length of the array, use [`ts_highlight_buffer_line_count`]. #[no_mangle] pub unsafe extern "C" fn ts_highlight_buffer_line_offsets( this: *const TSHighlightBuffer, @@ -186,12 +219,24 @@ pub unsafe extern "C" fn ts_highlight_buffer_line_offsets( this.renderer.line_offsets.as_slice().as_ptr() } +/// Get the length of the HTML content of a [`TSHighlightBuffer`] instance. +/// +/// # Safety +/// +/// `this` must be non-null and must be a valid pointer to a [`TSHighlightBuffer`] instance +/// created by [`ts_highlight_buffer_new`]. #[no_mangle] pub unsafe extern "C" fn ts_highlight_buffer_len(this: *const TSHighlightBuffer) -> u32 { let this = unwrap_ptr(this); this.renderer.html.len() as u32 } +/// Get the number of lines in a [`TSHighlightBuffer`] instance. +/// +/// # Safety +/// +/// `this` must be non-null and must be a valid pointer to a [`TSHighlightBuffer`] instance +/// created by [`ts_highlight_buffer_new`]. #[no_mangle] pub unsafe extern "C" fn ts_highlight_buffer_line_count(this: *const TSHighlightBuffer) -> u32 { let this = unwrap_ptr(this); @@ -202,8 +247,10 @@ pub unsafe extern "C" fn ts_highlight_buffer_line_count(this: *const TSHighlight /// /// # Safety /// -/// The caller must ensure that `scope_name`, `source_code`, and `cancellation_flag` are valid for +/// The caller must ensure that `scope_name`, `source_code`, `output`, and `cancellation_flag` are valid for /// the lifetime of the [`TSHighlighter`] instance, and are non-null. +/// +/// `this` must be a non-null pointer to a [`TSHighlighter`] instance created by [`ts_highlighter_new`] #[no_mangle] pub unsafe extern "C" fn ts_highlighter_highlight( this: *const TSHighlighter, diff --git a/tags/src/c_lib.rs b/tags/src/c_lib.rs index df21f181..915b0220 100644 --- a/tags/src/c_lib.rs +++ b/tags/src/c_lib.rs @@ -70,7 +70,7 @@ pub extern "C" fn ts_tagger_new() -> *mut TSTagger { /// /// # Safety /// -/// `this` must be non-null +/// `this` must be non-null and a valid pointer to a [`TSTagger`] instance. #[no_mangle] pub unsafe extern "C" fn ts_tagger_delete(this: *mut TSTagger) { drop(Box::from_raw(this)) @@ -78,9 +78,15 @@ pub unsafe extern "C" fn ts_tagger_delete(this: *mut TSTagger) { /// Add a language to a TSTagger. /// +/// Returns a [`TSTagsError`] indicating whether the operation was successful or not. +/// /// # Safety /// -/// `this` must be non-null +/// `this` must be non-null and a valid pointer to a [`TSTagger`] instance. +/// `scope_name` must be non-null and a valid pointer to a null-terminated string. +/// `tags_query` and `locals_query` must be non-null and valid pointers to strings. +/// +/// The caller must ensure that the lengths of `tags_query` and `locals_query` are correct. #[no_mangle] pub unsafe extern "C" fn ts_tagger_add_language( this: *mut TSTagger, @@ -94,7 +100,7 @@ pub unsafe extern "C" fn ts_tagger_add_language( let tagger = unwrap_mut_ptr(this); let scope_name = unwrap(CStr::from_ptr(scope_name).to_str()); let tags_query = slice::from_raw_parts(tags_query, tags_query_len as usize); - let locals_query = if locals_query != std::ptr::null() { + let locals_query = if !locals_query.is_null() { slice::from_raw_parts(locals_query, locals_query_len as usize) } else { &[] @@ -121,11 +127,17 @@ pub unsafe extern "C" fn ts_tagger_add_language( } } -/// Tag some source code. +/// Tags some source code. +/// +/// Returns a [`TSTagsError`] indicating whether the operation was successful or not. /// /// # Safety /// -/// `this` must be non-null +/// `this` must be a non-null valid pointer to a [`TSTagger`] instance. +/// `scope_name` must be a non-null valid pointer to a null-terminated string. +/// `source_code` must be a non-null valid pointer to a slice of bytes. +/// `output` must be a non-null valid pointer to a [`TSTagsBuffer`] instance. +/// `cancellation_flag` must be a non-null valid pointer to an [`AtomicUsize`] instance. #[no_mangle] pub unsafe extern "C" fn ts_tagger_tag( this: *mut TSTagger, @@ -220,36 +232,75 @@ pub extern "C" fn ts_tags_buffer_new() -> *mut TSTagsBuffer { /// /// # Safety /// -/// `this` must be non-null +/// `this` must be non-null and a valid pointer to a [`TSTagsBuffer`] instance created by +/// [`ts_tags_buffer_new`]. #[no_mangle] pub unsafe extern "C" fn ts_tags_buffer_delete(this: *mut TSTagsBuffer) { drop(Box::from_raw(this)) } +/// Get the tags from a TSTagsBuffer. +/// +/// # Safety +/// +/// `this` must be non-null and a valid pointer to a [`TSTagsBuffer`] instance created by +/// [`ts_tags_buffer_new`]. +/// +/// The caller must ensure that the returned pointer is not used after the [`TSTagsBuffer`] +/// is deleted with [`ts_tags_buffer_delete`], else the data will point to garbage. #[no_mangle] pub unsafe extern "C" fn ts_tags_buffer_tags(this: *const TSTagsBuffer) -> *const TSTag { let buffer = unwrap_ptr(this); buffer.tags.as_ptr() } +/// Get the number of tags in a TSTagsBuffer. +/// +/// # Safety +/// +/// `this` must be non-null and a valid pointer to a [`TSTagsBuffer`] instance. #[no_mangle] pub unsafe extern "C" fn ts_tags_buffer_tags_len(this: *const TSTagsBuffer) -> u32 { let buffer = unwrap_ptr(this); buffer.tags.len() as u32 } +/// Get the documentation strings from a TSTagsBuffer. +/// +/// # Safety +/// +/// `this` must be non-null and a valid pointer to a [`TSTagsBuffer`] instance created by +/// [`ts_tags_buffer_new`]. +/// +/// The caller must ensure that the returned pointer is not used after the [`TSTagsBuffer`] +/// is deleted with [`ts_tags_buffer_delete`], else the data will point to garbage. +/// +/// The returned pointer points to a C-style string. +/// To get the length of the string, use [`ts_tags_buffer_docs_len`]. #[no_mangle] pub unsafe extern "C" fn ts_tags_buffer_docs(this: *const TSTagsBuffer) -> *const c_char { let buffer = unwrap_ptr(this); buffer.docs.as_ptr() as *const c_char } +/// Get the length of the documentation strings in a TSTagsBuffer. +/// +/// # Safety +/// +/// `this` must be non-null and a valid pointer to a [`TSTagsBuffer`] instance created by +/// [`ts_tags_buffer_new`]. #[no_mangle] pub unsafe extern "C" fn ts_tags_buffer_docs_len(this: *const TSTagsBuffer) -> u32 { let buffer = unwrap_ptr(this); buffer.docs.len() as u32 } +/// Get whether or not a TSTagsBuffer contains any parse errors. +/// +/// # Safety +/// +/// `this` must be non-null and a valid pointer to a [`TSTagsBuffer`] instance created by +/// [`ts_tags_buffer_new`]. #[no_mangle] pub unsafe extern "C" fn ts_tags_buffer_found_parse_error(this: *const TSTagsBuffer) -> bool { let buffer = unwrap_ptr(this); @@ -258,9 +309,19 @@ pub unsafe extern "C" fn ts_tags_buffer_found_parse_error(this: *const TSTagsBuf /// Get the syntax kinds for a given scope name. /// +/// Returns a pointer to a null-terminated array of null-terminated strings. +/// /// # Safety /// -/// `this` must be non-null +/// `this` must be non-null and a valid pointer to a [`TSTagger`] instance created by +/// [`ts_tagger_new`]. +/// `scope_name` must be non-null and a valid pointer to a null-terminated string. +/// `len` must be non-null and a valid pointer to a `u32`. +/// +/// The caller must ensure that the returned pointer is not used after the [`TSTagger`] +/// is deleted with [`ts_tagger_delete`], else the data will point to garbage. +/// +/// The returned pointer points to a C-style string array. #[no_mangle] pub unsafe extern "C" fn ts_tagger_syntax_kinds_for_scope_name( this: *mut TSTagger, @@ -280,14 +341,14 @@ pub unsafe extern "C" fn ts_tagger_syntax_kinds_for_scope_name( } unsafe fn unwrap_ptr<'a, T>(result: *const T) -> &'a T { - unsafe { result.as_ref() }.unwrap_or_else(|| { + result.as_ref().unwrap_or_else(|| { eprintln!("{}:{} - pointer must not be null", file!(), line!()); abort(); }) } unsafe fn unwrap_mut_ptr<'a, T>(result: *mut T) -> &'a mut T { - unsafe { result.as_mut() }.unwrap_or_else(|| { + result.as_mut().unwrap_or_else(|| { eprintln!("{}:{} - pointer must not be null", file!(), line!()); abort(); })