From 957ab7c6e26fcdb766fe351597b1815df8609082 Mon Sep 17 00:00:00 2001 From: jsenkpiel-godaddy Date: Thu, 28 Jul 2022 12:58:18 -0700 Subject: [PATCH 1/4] meta: .gitignore more output files --- .gitignore | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.gitignore b/.gitignore index 767dae2..64fb695 100644 --- a/.gitignore +++ b/.gitignore @@ -8,3 +8,7 @@ Cargo.lock # These are backup files generated by rustfmt **/*.rs.bk + +node_modules/ +binaries/ +output/ From 5f3d5045f7e2a185596cf012893fb00589cb5736 Mon Sep 17 00:00:00 2001 From: jsenkpiel-godaddy Date: Thu, 28 Jul 2022 12:58:35 -0700 Subject: [PATCH 2/4] build: fix node tests --- libcobhandemo/build.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/libcobhandemo/build.sh b/libcobhandemo/build.sh index 055e8e3..6b01fc8 100755 --- a/libcobhandemo/build.sh +++ b/libcobhandemo/build.sh @@ -99,6 +99,7 @@ done ########## # Test Rust dynamic library file using node +mkdir -p node-test/libcobhandemo/binaries/ cp "target/${BUILD_DIR}/libcobhandemo${DYN_EXT}" "node-test/libcobhandemo/binaries/libcobhandemo-${DYN_SUFFIX}" npm -C node-test/libcobhandemo install pushd node-test/consumer-console-app From f950097447bb10479f61ffabe72b25bfa95b6c3a Mon Sep 17 00:00:00 2001 From: jsenkpiel-godaddy Date: Thu, 28 Jul 2022 12:58:58 -0700 Subject: [PATCH 3/4] demo: fix python demo --- libcobhandemo/python-test/cobhan_demo_lib/cobhan_demo.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libcobhandemo/python-test/cobhan_demo_lib/cobhan_demo.py b/libcobhandemo/python-test/cobhan_demo_lib/cobhan_demo.py index c7c8635..0fc84e3 100644 --- a/libcobhandemo/python-test/cobhan_demo_lib/cobhan_demo.py +++ b/libcobhandemo/python-test/cobhan_demo_lib/cobhan_demo.py @@ -16,13 +16,13 @@ class CobhanDemoLib(Cobhan): @classmethod def from_library_path(cls, library_root_path): instance = cls() - instance._load_library(library_root_path, 'libcobhandemo', CobhanDemoLib.CDEFINES) + instance.load_library(library_root_path, 'libcobhandemo', CobhanDemoLib.CDEFINES) return instance @classmethod def from_library_file(cls, library_file_path): instance = cls() - instance._load_library_direct(library_file_path, CobhanDemoLib.CDEFINES) + instance.load_library_direct(library_file_path, CobhanDemoLib.CDEFINES) return instance def to_upper(self, input): From b9a7beb45ce16b738fa8760b7a807de632ffa37a Mon Sep 17 00:00:00 2001 From: jsenkpiel-godaddy Date: Thu, 28 Jul 2022 13:10:49 -0700 Subject: [PATCH 4/4] refactor: many improvements Update based on rust 1.62 - rustfmt formatting - clippy linting - Use `.map_err()` instead of `match` where appropriate. - Removed some intermediately utf8 validation (i.e bytes->str->json is now bytes->json). - Use `temp_to_vector` where possible instead of `temp_to_string`. - Removed some unnecessary `memcopy`s - Avoid `.to_vec()`. - Uses `std::borrow::Cow` to avoid another vector allocation. - Uses implicit return when appropriate. --- cobhan/src/lib.rs | 254 ++++++++++++++++++++------------------- libcobhandemo/src/lib.rs | 59 +++++---- 2 files changed, 159 insertions(+), 154 deletions(-) diff --git a/cobhan/src/lib.rs b/cobhan/src/lib.rs index f6c22c1..87064ae 100644 --- a/cobhan/src/lib.rs +++ b/cobhan/src/lib.rs @@ -1,13 +1,15 @@ +use std::borrow::Cow; +use std::collections::HashMap; +use std::fs; +use std::io::Write; use std::os::raw::c_char; use std::ptr::copy_nonoverlapping; use std::slice::from_raw_parts; -use serde_json::{Value}; -use std::collections::HashMap; -use tempfile::NamedTempFile; -use std::io::{Write}; -use std::fs; use std::str; +use serde_json::Value; +use tempfile::NamedTempFile; + const ERR_NONE: i32 = 0; //One of the provided pointers is NULL / nil / 0 @@ -42,14 +44,16 @@ static MAXIMUM_BUFFER_SIZE: i32 = i32::MAX; #[cfg(feature = "cobhan_debug")] macro_rules! debug_print { - ($( $args:expr ),*) => { println!( $( $args ),* ); } + ($( $args:expr ),*) => { println!($($args ),*); }; } #[cfg(not(feature = "cobhan_debug"))] macro_rules! debug_print { - ($( $args:expr ),*) => {} + ($( $args:expr ),*) => {}; } +/// ## Safety +/// tbd pub unsafe fn cbuffer_to_vector(buffer: *const c_char) -> Result, i32> { if buffer.is_null() { debug_print!("cbuffer_to_vector: buffer is NULL"); @@ -61,8 +65,12 @@ pub unsafe fn cbuffer_to_vector(buffer: *const c_char) -> Result, i32> { debug_print!("cbuffer_to_vector: raw length field is {}", length); if length > MAXIMUM_BUFFER_SIZE { - debug_print!("cbuffer_to_vector: length {} is larger than MAXIMUM_BUFFER_SIZE ({})", length, MAXIMUM_BUFFER_SIZE); - return Err(ERR_BUFFER_TOO_LARGE) + debug_print!( + "cbuffer_to_vector: length {} is larger than MAXIMUM_BUFFER_SIZE ({})", + length, + MAXIMUM_BUFFER_SIZE + ); + return Err(ERR_BUFFER_TOO_LARGE); } if length < 0 { debug_print!("cbuffer_to_vector: calling temp_to_vector"); @@ -73,25 +81,8 @@ pub unsafe fn cbuffer_to_vector(buffer: *const c_char) -> Result, i32> { Ok(from_raw_parts(payload, length as usize).to_vec()) } -unsafe fn temp_to_vector(payload: *const u8, length: i32) -> Result, i32> { - let file_name; - match str::from_utf8(from_raw_parts(payload, (0 - length) as usize)) { - Ok(f) => file_name = f, - Err(..) => { - debug_print!("temp_to_vector: temp file name is invalid utf-8 string (length = {})", 0 - length); - return Err(ERR_INVALID_UTF8); - } - } - - return match fs::read(file_name) { - Ok(s) => Ok(s), - Err(_e) => { - debug_print!("temp_to_vector: failed to read temporary file {}: {}", file_name, _e); - Err(ERR_READ_TEMP_FILE_FAILED) - } - } -} - +/// ## Safety +/// tbd pub unsafe fn cbuffer_to_string(buffer: *const c_char) -> Result { if buffer.is_null() { debug_print!("cbuffer_to_string: buffer is NULL"); @@ -103,7 +94,11 @@ pub unsafe fn cbuffer_to_string(buffer: *const c_char) -> Result { debug_print!("cbuffer_to_string: raw length field is {}", length); if length > MAXIMUM_BUFFER_SIZE { - debug_print!("cbuffer_to_string: length {} is larger than MAXIMUM_BUFFER_SIZE ({})", length, MAXIMUM_BUFFER_SIZE); + debug_print!( + "cbuffer_to_string: length {} is larger than MAXIMUM_BUFFER_SIZE ({})", + length, + MAXIMUM_BUFFER_SIZE + ); return Err(ERR_BUFFER_TOO_LARGE); } @@ -111,41 +106,67 @@ pub unsafe fn cbuffer_to_string(buffer: *const c_char) -> Result { if length < 0 { debug_print!("cbuffer_to_string: calling temp_to_string"); - return temp_to_string(payload, length) + return temp_to_string(payload, length); } - //Allocation: to_vec() is a clone/copy - return match String::from_utf8(from_raw_parts(payload, length as usize).to_vec()) { - Ok(input_str) => Ok(input_str), - Err(..) => { - debug_print!("cbuffer_to_string: payload is invalid utf-8 string (length = {})", length); - Err(ERR_INVALID_UTF8) - } - } + str::from_utf8(from_raw_parts(payload, length as usize)) + .map(|s| s.to_owned()) + .map_err(|_| { + debug_print!( + "cbuffer_to_string: payload is invalid utf-8 string (length = {})", + length + ); + ERR_INVALID_UTF8 + }) } unsafe fn temp_to_string(payload: *const u8, length: i32) -> Result { - let file_name; - match str::from_utf8(from_raw_parts(payload, (0 - length) as usize)) { - Ok(f) => file_name = f, - Err(..) => { - debug_print!("temp_to_string: temp file name is invalid utf-8 string (length = {})", 0 - length); - return Err(ERR_INVALID_UTF8); - } - } + let file_name = + str::from_utf8(from_raw_parts(payload, (0 - length) as usize)).map_err(|_| { + debug_print!( + "temp_to_string: temp file name is invalid utf-8 string (length = {})", + 0 - length + ); + ERR_INVALID_UTF8 + })?; debug_print!("temp_to_string: reading temp file {}", file_name); - return match fs::read_to_string(file_name) { - Ok(s) => Ok(s), - Err(_e) => { - debug_print!("temp_to_string: Error reading temp file {}: {}", file_name, _e); - Err(ERR_READ_TEMP_FILE_FAILED) - } - } + fs::read_to_string(file_name).map_err(|_e| { + debug_print!( + "temp_to_string: Error reading temp file {}: {}", + file_name, + _e + ); + ERR_READ_TEMP_FILE_FAILED + }) +} + +unsafe fn temp_to_vector(payload: *const u8, length: i32) -> Result, i32> { + let file_name = + str::from_utf8(from_raw_parts(payload, (0 - length) as usize)).map_err(|_| { + debug_print!( + "temp_to_vector: temp file name is invalid utf-8 string (length = {})", + 0 - length + ); + ERR_INVALID_UTF8 + })?; + + fs::read(file_name).map_err(|_e| { + debug_print!( + "temp_to_vector: failed to read temporary file {}: {}", + file_name, + _e + ); + ERR_READ_TEMP_FILE_FAILED + }) } -pub unsafe fn cbuffer_to_hashmap_json(buffer: *const c_char) -> Result, i32> { +/// ## Safety +/// tbd +pub unsafe fn cbuffer_to_hashmap_json( + buffer: *const c_char, +) -> Result, i32> { if buffer.is_null() { debug_print!("cbuffer_to_hashmap_json: buffer is NULL"); return Err(ERR_NULL_PTR); @@ -155,56 +176,50 @@ pub unsafe fn cbuffer_to_hashmap_json(buffer: *const c_char) -> Result MAXIMUM_BUFFER_SIZE { - debug_print!("cbuffer_to_hashmap_json: length {} is larger than MAXIMUM_BUFFER_SIZE ({})", length, MAXIMUM_BUFFER_SIZE); + debug_print!( + "cbuffer_to_hashmap_json: length {} is larger than MAXIMUM_BUFFER_SIZE ({})", + length, + MAXIMUM_BUFFER_SIZE + ); return Err(ERR_BUFFER_TOO_LARGE); } debug_print!("cbuffer_to_hashmap_json: raw length field is {}", length); - let json_str; - let temp_string_result; - if length >= 0 { - match str::from_utf8(from_raw_parts(payload, length as usize)) { - Ok(input_str) => json_str = input_str, - Err(..) => { - debug_print!("cbuffer_to_hashmap_json: payload is invalid utf-8 string (length = {})", length); - return Err(ERR_INVALID_UTF8); - } - } + let json_bytes = if length >= 0 { + Cow::Borrowed(from_raw_parts(payload, length as usize)) } else { - debug_print!("cbuffer_to_hashmap_json: calling temp_to_string"); - match temp_to_string(payload, length) { - Ok(string) => { - temp_string_result = string; - json_str = &temp_string_result; - }, - Err(e) => return Err(e) - } - } - - return match serde_json::from_str(&json_str) { - Ok(json) => Ok(json), - Err(_e) => { - debug_print!("cbuffer_to_hashmap_json: serde_json::from_str / JSON decode failed {}", _e); - Err(ERR_JSON_DECODE_FAILED) - } - } + debug_print!("cbuffer_to_hashmap_json: calling temp_to_vector"); + Cow::Owned(temp_to_vector(payload, length)?) + }; + + serde_json::from_slice(&json_bytes).map_err(|_e| { + debug_print!( + "cbuffer_to_hashmap_json: serde_json::from_slice / JSON decode failed {}", + _e + ); + ERR_JSON_DECODE_FAILED + }) } -pub unsafe fn hashmap_json_to_cbuffer(json: &HashMap, buffer: *mut c_char) -> i32 { - //TODO: Use tovec? - return match serde_json::to_string(&json) { - Ok(json_str) => string_to_cbuffer(&json_str, buffer), - Err(..) => ERR_JSON_ENCODE_FAILED +/// ## Safety +/// tbd +pub unsafe fn hashmap_json_to_cbuffer(json: &HashMap, buffer: *mut c_char) -> i32 { + match serde_json::to_vec(&json) { + Ok(json_bytes) => bytes_to_cbuffer(&json_bytes, buffer), + Err(_) => ERR_JSON_ENCODE_FAILED, } } +/// ## Safety +/// tbd pub unsafe fn string_to_cbuffer(string: &str, buffer: *mut c_char) -> i32 { bytes_to_cbuffer(string.as_bytes(), buffer) } +/// ## Safety +/// tbd pub unsafe fn bytes_to_cbuffer(bytes: &[u8], buffer: *mut c_char) -> i32 { if buffer.is_null() { debug_print!("bytes_to_cbuffer: buffer is NULL"); @@ -239,12 +254,16 @@ pub unsafe fn bytes_to_cbuffer(bytes: &[u8], buffer: *mut c_char) -> i32 { } unsafe fn bytes_to_temp(bytes: &[u8], buffer: *mut c_char) -> i32 { - let tmp_file_path; - match write_new_file(bytes) { - Ok(t) => tmp_file_path = t, - Err(r) => return r - } - debug_print!("bytes_to_temp: write_new_file wrote {} bytes to {}", bytes.len(), tmp_file_path); + // TODO: eventually replace this pattern with if-let once that is stable -jsenkpiel + let tmp_file_path = match write_new_file(bytes) { + Ok(t) => t, + Err(r) => return r, + }; + debug_print!( + "bytes_to_temp: write_new_file wrote {} bytes to {}", + bytes.len(), + tmp_file_path + ); let length = buffer as *mut i32; let tmp_file_path_len = tmp_file_path.len() as i32; @@ -252,51 +271,40 @@ unsafe fn bytes_to_temp(bytes: &[u8], buffer: *mut c_char) -> i32 { //NOTE: We explicitly test this so we don't recursively attempt to create temp files with string_to_cbuffer() if *length < tmp_file_path_len { //Temp file path won't fit in output buffer, we're out of luck - debug_print!("bytes_to_temp: temp file path {} is larger than buffer capacity {}", tmp_file_path, *length); + debug_print!( + "bytes_to_temp: temp file path {} is larger than buffer capacity {}", + tmp_file_path, + *length + ); let _ = fs::remove_file(tmp_file_path); return ERR_BUFFER_TOO_SMALL; } let result = string_to_cbuffer(&tmp_file_path, buffer); if result != ERR_NONE { - debug_print!("bytes_to_temp: failed to store temp path {} in buffer", tmp_file_path); + debug_print!( + "bytes_to_temp: failed to store temp path {} in buffer", + tmp_file_path + ); let _ = fs::remove_file(tmp_file_path); return result; } *length = 0 - tmp_file_path_len; - return result; + + result } unsafe fn write_new_file(bytes: &[u8]) -> Result { - let mut tmpfile; - match NamedTempFile::new() { - Ok(f) => tmpfile = f, - Err(..) => return Err(ERR_WRITE_TEMP_FILE_FAILED) - } - let bytes_len = bytes.len(); - - let bytes_written; - match tmpfile.write_all(&bytes) { - Ok(..) => bytes_written = bytes_len as i32, - Err(..) => return Err(ERR_WRITE_TEMP_FILE_FAILED) - } + let mut tmpfile = NamedTempFile::new().map_err(|_| ERR_WRITE_TEMP_FILE_FAILED)?; - if (bytes_written as usize) != bytes_len { + if tmpfile.write_all(bytes).is_err() { return Err(ERR_WRITE_TEMP_FILE_FAILED); - } + }; - let result; - match tmpfile.keep() { - Ok(r) => result = r, - Err(..) => return Err(ERR_WRITE_TEMP_FILE_FAILED) - } - let (_, path) = result; + let (_, path) = tmpfile.keep().map_err(|_| ERR_WRITE_TEMP_FILE_FAILED)?; - return match path.into_os_string().into_string() { - Ok(tmp_path) => Ok(tmp_path), - Err(..) => { - Err(ERR_WRITE_TEMP_FILE_FAILED) - } - } + path.into_os_string() + .into_string() + .map_err(|_| ERR_WRITE_TEMP_FILE_FAILED) } diff --git a/libcobhandemo/src/lib.rs b/libcobhandemo/src/lib.rs index 6210509..3ae0ba4 100644 --- a/libcobhandemo/src/lib.rs +++ b/libcobhandemo/src/lib.rs @@ -1,20 +1,21 @@ +#![allow(clippy::missing_safety_doc)] + +use std::collections::HashMap; use std::os::raw::c_char; +use std::sync::atomic::{AtomicI32, Ordering}; use std::{thread, time}; -use serde_json::{Value}; -use std::collections::HashMap; + use rand::Rng; use rand::RngCore; -use std::sync::atomic::{AtomicI32, Ordering}; +use serde_json::Value; static COUNTER: AtomicI32 = AtomicI32::new(0); #[no_mangle] pub unsafe extern "C" fn spawnThread() { - std::thread::spawn(move || { - loop { - COUNTER.fetch_add(1, Ordering::Relaxed); - thread::sleep(time::Duration::from_secs(1)) - } + std::thread::spawn(move || loop { + COUNTER.fetch_add(1, Ordering::Relaxed); + thread::sleep(time::Duration::from_secs(1)) }); } @@ -45,11 +46,10 @@ pub unsafe extern "C" fn addDouble(x: f64, y: f64) -> f64 { #[no_mangle] pub unsafe extern "C" fn toUpper(input: *const c_char, output: *mut c_char) -> i32 { - let input_str; - match cobhan::cbuffer_to_string(input) { - Ok(str) => input_str = str, - Err(e) => return e - } + let input_str = match cobhan::cbuffer_to_string(input) { + Ok(s) => s, + Err(e) => return e, + }; let output_str = input_str.to_uppercase(); @@ -57,18 +57,21 @@ pub unsafe extern "C" fn toUpper(input: *const c_char, output: *mut c_char) -> i } #[no_mangle] -pub unsafe extern "C" fn filterJson(input: *const c_char, disallowed_value: *const c_char, output: *mut c_char) -> i32 { +pub unsafe extern "C" fn filterJson( + input: *const c_char, + disallowed_value: *const c_char, + output: *mut c_char, +) -> i32 { let mut json; match cobhan::cbuffer_to_hashmap_json(input) { Ok(input_json) => json = input_json, - Err(e) => return e + Err(e) => return e, } - let disallowed_value_str; - match cobhan::cbuffer_to_string(disallowed_value) { - Ok(disallow) => disallowed_value_str = disallow, - Err(e) => return e - } + let disallowed_value_str = match cobhan::cbuffer_to_string(disallowed_value) { + Ok(disallow) => disallow, + Err(e) => return e, + }; filter_json(&mut json, &disallowed_value_str); @@ -77,21 +80,15 @@ pub unsafe extern "C" fn filterJson(input: *const c_char, disallowed_value: *con // Example of a safe function pub fn filter_json(json: &mut HashMap, disallowed: &str) { - json.retain(|_key, value| { - return match value.as_str() { - None => true, - v => !v.unwrap().contains(&disallowed) - } - }); + json.retain(|_key, value| matches!(value, Value::String(s) if !s.contains(&disallowed))); } #[no_mangle] pub unsafe extern "C" fn base64Encode(input: *const c_char, output: *mut c_char) -> i32 { - let bytes; - match cobhan::cbuffer_to_vector(input) { - Ok(b) => bytes = b, - Err(e) => return e - } + let bytes = match cobhan::cbuffer_to_vector(input) { + Ok(b) => b, + Err(e) => return e, + }; let b64str = base64::encode(bytes);