From 017fda070b86844d4860041a6e42ab12d7588836 Mon Sep 17 00:00:00 2001 From: William Brown Date: Jul 02 2020 23:24:44 +0000 Subject: Ticket 51175 - resolve plugin name leaking Bug Description: Previously pblock.c assumed that all plugin names were static c strings. Rust can't create static C strings, so these were intentionally leaked. Fix Description: Rather than leak these, we do a dup/free through the slapiplugin struct instead, meaning we can use ephemeral, and properly managed strings in rust. This does not affect any other existing code which will still handle the static strings correctly. https://pagure.io/389-ds-base/issue/51175 Author: William Brown Review by: mreynolds, tbordaz (Thanks!) --- diff --git a/Makefile.am b/Makefile.am index 3f21645..4ea239a 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1315,6 +1315,7 @@ rust-nsslapd-private.h: @abs_top_builddir@/rs/@rust_target_dir@/librnsslapd.a libslapi_r_plugin_SOURCES = \ src/slapi_r_plugin/src/backend.rs \ src/slapi_r_plugin/src/ber.rs \ + src/slapi_r_plugin/src/charray.rs \ src/slapi_r_plugin/src/constants.rs \ src/slapi_r_plugin/src/dn.rs \ src/slapi_r_plugin/src/entry.rs \ diff --git a/configure.ac b/configure.ac index 4843a82..8a029ec 100644 --- a/configure.ac +++ b/configure.ac @@ -131,7 +131,7 @@ if test "$enable_debug" = yes ; then debug_defs="-DDEBUG -DMCC_DEBUG" debug_cflags="-g3 -O0 -rdynamic" debug_cxxflags="-g3 -O0 -rdynamic" - debug_rust_defs="-C debuginfo=2" + debug_rust_defs="-C debuginfo=2 -Z macro-backtrace" cargo_defs="" rust_target_dir="debug" else diff --git a/dirsrvtests/tests/suites/basic/basic_test.py b/dirsrvtests/tests/suites/basic/basic_test.py index c85dc4b..47ba90a 100644 --- a/dirsrvtests/tests/suites/basic/basic_test.py +++ b/dirsrvtests/tests/suites/basic/basic_test.py @@ -251,11 +251,11 @@ def test_basic_import_export(topology_st, import_example_ldif): """ log.info('Running test_basic_import_export...') - # # Test online/offline LDIF imports # topology_st.standalone.start() + # topology_st.standalone.config.set('nsslapd-errorlog-level', '1') # Generate a test ldif (50k entries) log.info("Generating LDIF...") @@ -263,6 +263,7 @@ def test_basic_import_export(topology_st, import_example_ldif): import_ldif = ldif_dir + '/basic_import.ldif' dbgen_users(topology_st.standalone, 50000, import_ldif, DEFAULT_SUFFIX) + # Online log.info("Importing LDIF online...") import_task = ImportTask(topology_st.standalone) diff --git a/ldap/servers/slapd/pagedresults.c b/ldap/servers/slapd/pagedresults.c index d8b8798..e3444e9 100644 --- a/ldap/servers/slapd/pagedresults.c +++ b/ldap/servers/slapd/pagedresults.c @@ -738,10 +738,10 @@ pagedresults_cleanup(Connection *conn, int needlock) int i; PagedResults *prp = NULL; - slapi_log_err(SLAPI_LOG_TRACE, "pagedresults_cleanup", "=>\n"); + /* slapi_log_err(SLAPI_LOG_TRACE, "pagedresults_cleanup", "=>\n"); */ if (NULL == conn) { - slapi_log_err(SLAPI_LOG_TRACE, "pagedresults_cleanup", "<= Connection is NULL\n"); + /* slapi_log_err(SLAPI_LOG_TRACE, "pagedresults_cleanup", "<= Connection is NULL\n"); */ return 0; } @@ -767,7 +767,7 @@ pagedresults_cleanup(Connection *conn, int needlock) if (needlock) { pthread_mutex_unlock(&(conn->c_mutex)); } - slapi_log_err(SLAPI_LOG_TRACE, "pagedresults_cleanup", "<= %d\n", rc); + /* slapi_log_err(SLAPI_LOG_TRACE, "pagedresults_cleanup", "<= %d\n", rc); */ return rc; } diff --git a/ldap/servers/slapd/pblock.c b/ldap/servers/slapd/pblock.c index cb562e9..7a9578d 100644 --- a/ldap/servers/slapd/pblock.c +++ b/ldap/servers/slapd/pblock.c @@ -3341,13 +3341,15 @@ slapi_pblock_set(Slapi_PBlock *pblock, int arg, void *value) if (pblock->pb_plugin->plg_type != SLAPI_PLUGIN_SYNTAX) { return (-1); } - pblock->pb_plugin->plg_syntax_names = (char **)value; + PR_ASSERT(pblock->pb_plugin->plg_syntax_names == NULL); + pblock->pb_plugin->plg_syntax_names = slapi_ch_array_dup((char **)value); break; case SLAPI_PLUGIN_SYNTAX_OID: if (pblock->pb_plugin->plg_type != SLAPI_PLUGIN_SYNTAX) { return (-1); } - pblock->pb_plugin->plg_syntax_oid = (char *)value; + PR_ASSERT(pblock->pb_plugin->plg_syntax_oid == NULL); + pblock->pb_plugin->plg_syntax_oid = slapi_ch_strdup((char *)value); break; case SLAPI_PLUGIN_SYNTAX_FLAGS: if (pblock->pb_plugin->plg_type != SLAPI_PLUGIN_SYNTAX) { @@ -3796,7 +3798,8 @@ slapi_pblock_set(Slapi_PBlock *pblock, int arg, void *value) if (pblock->pb_plugin->plg_type != SLAPI_PLUGIN_MATCHINGRULE) { return (-1); } - pblock->pb_plugin->plg_mr_names = (char **)value; + PR_ASSERT(pblock->pb_plugin->plg_mr_names == NULL); + pblock->pb_plugin->plg_mr_names = slapi_ch_array_dup((char **)value); break; case SLAPI_PLUGIN_MR_COMPARE: if (pblock->pb_plugin->plg_type != SLAPI_PLUGIN_MATCHINGRULE) { diff --git a/ldap/servers/slapd/plugin.c b/ldap/servers/slapd/plugin.c index 282b987..e6b48de 100644 --- a/ldap/servers/slapd/plugin.c +++ b/ldap/servers/slapd/plugin.c @@ -2694,6 +2694,13 @@ plugin_free(struct slapdplugin *plugin) if (plugin->plg_type == SLAPI_PLUGIN_PWD_STORAGE_SCHEME || plugin->plg_type == SLAPI_PLUGIN_REVER_PWD_STORAGE_SCHEME) { slapi_ch_free_string(&plugin->plg_pwdstorageschemename); } + if (plugin->plg_type == SLAPI_PLUGIN_SYNTAX) { + slapi_ch_free_string(&plugin->plg_syntax_oid); + slapi_ch_array_free(plugin->plg_syntax_names); + } + if (plugin->plg_type == SLAPI_PLUGIN_MATCHINGRULE) { + slapi_ch_array_free(plugin->plg_mr_names); + } release_componentid(plugin->plg_identity); slapi_counter_destroy(&plugin->plg_op_counter); if (!plugin->plg_group) { diff --git a/ldap/servers/slapd/pw_verify.c b/ldap/servers/slapd/pw_verify.c index 4f0944b..4ff1fa2 100644 --- a/ldap/servers/slapd/pw_verify.c +++ b/ldap/servers/slapd/pw_verify.c @@ -111,6 +111,7 @@ pw_verify_token_dn(Slapi_PBlock *pb) { if (fernet_verify_token(dn, cred->bv_val, key, tok_ttl) != 0) { rc = SLAPI_BIND_SUCCESS; } + slapi_ch_free_string(&key); #endif return rc; } diff --git a/ldap/servers/slapd/tools/pwenc.c b/ldap/servers/slapd/tools/pwenc.c index 1629c06..d89225e 100644 --- a/ldap/servers/slapd/tools/pwenc.c +++ b/ldap/servers/slapd/tools/pwenc.c @@ -34,7 +34,7 @@ int ldap_syslog; int ldap_syslog_level; -int slapd_ldap_debug = LDAP_DEBUG_ANY; +/* int slapd_ldap_debug = LDAP_DEBUG_ANY; */ int detached; FILE *error_logfp; FILE *access_logfp; diff --git a/src/slapi_r_plugin/README.md b/src/slapi_r_plugin/README.md index af9743e..1c9bcbf 100644 --- a/src/slapi_r_plugin/README.md +++ b/src/slapi_r_plugin/README.md @@ -15,7 +15,7 @@ the [Rust Nomicon](https://doc.rust-lang.org/nomicon/index.html) > warning about danger. This document will not detail the specifics of unsafe or the invariants you must adhere to for rust -to work with C. +to work with C. Failure to uphold these invariants will lead to less than optimal consequences. If you still want to see more about the plugin bindings, go on ... @@ -135,7 +135,7 @@ associated functions. Now, you may notice that not all members of the trait are implemented. This is due to a feature of rust known as default trait impls. This allows the trait origin (src/plugin.rs) to provide template versions of these functions. If you "overwrite" them, your implementation is used. Unlike -OO, you may not inherit or call the default function. +OO, you may not inherit or call the default function. If a default is not provided you *must* implement that function to be considered valid. Today (20200422) this only applies to `start` and `close`. @@ -183,7 +183,7 @@ It's important to understand how Rust manages memory both on the stack and the h As a result, this means that we must express in code, assertions about the proper ownership of memory and who is responsible for it (unlike C, where it can be hard to determine who or what is responsible for freeing some value.) Failure to handle this correctly, can and will lead to crashes, leaks or -*hand waving* magical failures that are eXtReMeLy FuN to debug. +*hand waving* magical failures that are `eXtReMeLy FuN` to debug. ### Reference Types diff --git a/src/slapi_r_plugin/src/charray.rs b/src/slapi_r_plugin/src/charray.rs new file mode 100644 index 0000000..d2e4469 --- /dev/null +++ b/src/slapi_r_plugin/src/charray.rs @@ -0,0 +1,32 @@ +use std::ffi::CString; +use std::iter::once; +use std::os::raw::c_char; +use std::ptr; + +pub struct Charray { + pin: Vec, + charray: Vec<*const c_char>, +} + +impl Charray { + pub fn new(input: &[&str]) -> Result { + let pin: Result, ()> = input + .iter() + .map(|s| CString::new(*s).map_err(|_e| ())) + .collect(); + + let pin = pin?; + + let charray: Vec<_> = pin + .iter() + .map(|s| s.as_ptr()) + .chain(once(ptr::null())) + .collect(); + + Ok(Charray { pin, charray }) + } + + pub fn as_ptr(&self) -> *const *const c_char { + self.charray.as_ptr() + } +} diff --git a/src/slapi_r_plugin/src/lib.rs b/src/slapi_r_plugin/src/lib.rs index d7fc22e..e589057 100644 --- a/src/slapi_r_plugin/src/lib.rs +++ b/src/slapi_r_plugin/src/lib.rs @@ -1,9 +1,11 @@ -// extern crate lazy_static; +#[macro_use] +extern crate lazy_static; #[macro_use] pub mod macros; pub mod backend; pub mod ber; +pub mod charray; mod constants; pub mod dn; pub mod entry; @@ -19,6 +21,7 @@ pub mod value; pub mod prelude { pub use crate::backend::{BackendRef, BackendRefTxn}; pub use crate::ber::BerValRef; + pub use crate::charray::Charray; pub use crate::constants::{FilterType, PluginFnType, PluginType, PluginVersion, LDAP_SUCCESS}; pub use crate::dn::{Sdn, SdnRef}; pub use crate::entry::EntryRef; @@ -28,8 +31,7 @@ pub mod prelude { pub use crate::plugin::{register_plugin_ext, PluginIdRef, SlapiPlugin3}; pub use crate::search::{Search, SearchScope}; pub use crate::syntax_plugin::{ - matchingrule_register, name_to_leaking_char, names_to_leaking_char_array, SlapiOrdMr, - SlapiSubMr, SlapiSyntaxPlugin1, + matchingrule_register, SlapiOrdMr, SlapiSubMr, SlapiSyntaxPlugin1, }; pub use crate::task::{task_register_handler_fn, task_unregister_handler_fn, Task, TaskRef}; pub use crate::value::{Value, ValueArray, ValueArrayRef, ValueRef}; diff --git a/src/slapi_r_plugin/src/macros.rs b/src/slapi_r_plugin/src/macros.rs index 0304496..a185470 100644 --- a/src/slapi_r_plugin/src/macros.rs +++ b/src/slapi_r_plugin/src/macros.rs @@ -249,6 +249,7 @@ macro_rules! slapi_r_syntax_plugin_hooks { paste::item! { use libc; use std::convert::TryFrom; + use std::ffi::CString; #[no_mangle] pub extern "C" fn [<$mod_ident _plugin_init>](raw_pb: *const libc::c_void) -> i32 { @@ -261,15 +262,15 @@ macro_rules! slapi_r_syntax_plugin_hooks { }; // Setup the names/oids that this plugin provides syntaxes for. - - let name_ptr = unsafe { names_to_leaking_char_array(&$hooks_ident::attr_supported_names()) }; - match pb.register_syntax_names(name_ptr) { + // DS will clone these, so they can be ephemeral to this function. + let name_vec = Charray::new($hooks_ident::attr_supported_names().as_slice()).expect("invalid supported names"); + match pb.register_syntax_names(name_vec.as_ptr()) { 0 => {}, e => return e, }; - let name_ptr = unsafe { name_to_leaking_char($hooks_ident::attr_oid()) }; - match pb.register_syntax_oid(name_ptr) { + let attr_oid = CString::new($hooks_ident::attr_oid()).expect("invalid attr oid"); + match pb.register_syntax_oid(attr_oid.as_ptr()) { 0 => {}, e => return e, }; @@ -430,7 +431,8 @@ macro_rules! slapi_r_syntax_plugin_hooks { e => return e, }; - let name_ptr = unsafe { names_to_leaking_char_array(&$hooks_ident::eq_mr_supported_names()) }; + let name_vec = Charray::new($hooks_ident::eq_mr_supported_names().as_slice()).expect("invalid mr supported names"); + let name_ptr = name_vec.as_ptr(); // SLAPI_PLUGIN_MR_NAMES match pb.register_mr_names(name_ptr) { 0 => {}, @@ -672,7 +674,8 @@ macro_rules! slapi_r_syntax_plugin_hooks { e => return e, }; - let name_ptr = unsafe { names_to_leaking_char_array(&$hooks_ident::ord_mr_supported_names()) }; + let name_vec = Charray::new($hooks_ident::ord_mr_supported_names().as_slice()).expect("invalid ord supported names"); + let name_ptr = name_vec.as_ptr(); // SLAPI_PLUGIN_MR_NAMES match pb.register_mr_names(name_ptr) { 0 => {}, diff --git a/src/slapi_r_plugin/src/syntax_plugin.rs b/src/slapi_r_plugin/src/syntax_plugin.rs index e7d5c01..86f84bd 100644 --- a/src/slapi_r_plugin/src/syntax_plugin.rs +++ b/src/slapi_r_plugin/src/syntax_plugin.rs @@ -1,11 +1,11 @@ use crate::ber::BerValRef; // use crate::constants::FilterType; +use crate::charray::Charray; use crate::error::PluginError; use crate::pblock::PblockRef; use crate::value::{ValueArray, ValueArrayRef}; use std::cmp::Ordering; use std::ffi::CString; -use std::iter::once; use std::os::raw::c_char; use std::ptr; @@ -26,37 +26,6 @@ struct slapi_matchingRuleEntry { mr_compat_syntax: *const *const c_char, } -pub unsafe fn name_to_leaking_char(name: &str) -> *const c_char { - let n = CString::new(name) - .expect("An invalid string has been hardcoded!") - .into_boxed_c_str(); - let n_ptr = n.as_ptr(); - // Now we intentionally leak the name here, and the pointer will remain valid. - Box::leak(n); - n_ptr -} - -pub unsafe fn names_to_leaking_char_array(names: &[&str]) -> *const *const c_char { - let n_arr: Vec = names - .iter() - .map(|s| CString::new(*s).expect("An invalid string has been hardcoded!")) - .collect(); - let n_arr = n_arr.into_boxed_slice(); - let n_ptr_arr: Vec<*const c_char> = n_arr - .iter() - .map(|v| v.as_ptr()) - .chain(once(ptr::null())) - .collect(); - let n_ptr_arr = n_ptr_arr.into_boxed_slice(); - - // Now we intentionally leak these names here, - let _r_n_arr = Box::leak(n_arr); - let r_n_ptr_arr = Box::leak(n_ptr_arr); - - let name_ptr = r_n_ptr_arr as *const _ as *const *const c_char; - name_ptr -} - // oid - the oid of the matching rule // name - the name of the mr // desc - description @@ -69,20 +38,24 @@ pub unsafe fn matchingrule_register( syntax: &str, compat_syntax: &[&str], ) -> i32 { - let oid_ptr = name_to_leaking_char(oid); - let name_ptr = name_to_leaking_char(name); - let desc_ptr = name_to_leaking_char(desc); - let syntax_ptr = name_to_leaking_char(syntax); - let compat_syntax_ptr = names_to_leaking_char_array(compat_syntax); + // Make everything CStrings that live long enough. + + let oid_cs = CString::new(oid).expect("invalid oid"); + let name_cs = CString::new(name).expect("invalid name"); + let desc_cs = CString::new(desc).expect("invalid desc"); + let syntax_cs = CString::new(syntax).expect("invalid syntax"); + + // We have to do this so the cstrings live long enough. + let compat_syntax_ca = Charray::new(compat_syntax).expect("invalid compat_syntax"); let new_mr = slapi_matchingRuleEntry { - mr_oid: oid_ptr, + mr_oid: oid_cs.as_ptr(), _mr_oidalias: ptr::null(), - mr_name: name_ptr, - mr_desc: desc_ptr, - mr_syntax: syntax_ptr, + mr_name: name_cs.as_ptr(), + mr_desc: desc_cs.as_ptr(), + mr_syntax: syntax_cs.as_ptr(), _mr_obsolete: 0, - mr_compat_syntax: compat_syntax_ptr, + mr_compat_syntax: compat_syntax_ca.as_ptr(), }; let new_mr_ptr = &new_mr as *const _;