Code cleanup and fixes (#300)

* remove cpp warning

* code clean app

* bump version

* bug fix: call_sender_principal matches the device_principal

* fix spelling errors

Author: abenso

CMakeLists.txt

@@ -13,11 +13,11 @@
 #*  See the License for the specific language governing permissions and
 #*  limitations under the License.
 #********************************************************************************
-cmake_minimum_required(VERSION 3.0)
+cmake_minimum_required(VERSION 3.28)
 include("cmake/HunterGate.cmake")
 HunterGate(
-    URL "https://github.com/cpp-pm/hunter/archive/v0.25.5.tar.gz"
-    SHA1 "a20151e4c0740ee7d0f9994476856d813cdead29"
+    URL "https://github.com/cpp-pm/hunter/archive/v0.26.1.tar.gz"
+    SHA1 "e41ac7a18c49b35ebac99ff2b5244317b2638a65"
     LOCAL
 )
 
@@ -42,8 +42,16 @@ add_definitions(-DAPP_STANDARD)
 
 hunter_add_package(fmt)
 find_package(fmt CONFIG REQUIRED)
-hunter_add_package(jsoncpp)
-find_package(jsoncpp CONFIG REQUIRED)
+
+# Use FetchContent to get nlohmann_json 3.12.0 since Hunter doesn't support this version
+include(FetchContent)
+FetchContent_Declare(
+    nlohmann_json
+    URL https://github.com/nlohmann/json/releases/download/v3.12.0/json.tar.xz
+    URL_HASH SHA256=42f6e95cad6ec532fd372391373363b62a14af6d771056dbfc86160e6dfff7aa
+)
+FetchContent_MakeAvailable(nlohmann_json)
+
 hunter_add_package(GTest)
 find_package(GTest CONFIG REQUIRED)
 
@@ -165,7 +173,7 @@ target_link_libraries(unittests PRIVATE
         GTest::gtest_main
         app_lib
         fmt::fmt
-        JsonCpp::JsonCpp)
+        nlohmann_json::nlohmann_json)
 
 add_compile_definitions(TESTVECTORS_DIR="${CMAKE_CURRENT_SOURCE_DIR}/tests/")
 add_test(NAME unittests COMMAND unittests)

README.md

@@ -156,7 +156,7 @@ cd zemu
 yarn test -t 'test name'
 ```
 
-This will run just the test maching the name provided
+This will run just the test matching the name provided
 
 ## How to debug a ledger app?
 
@@ -199,7 +199,7 @@ There are a few things to take into account when enabling Ledger App debugging:
 
 3. Launch the emulator in debug mode
 
-    > If you didnt install Zemu yet (previous section), then run `make zemu_install`
+    > If you did not install Zemu yet (previous section), then run `make zemu_install`
 
     ```sh
     make zemu_debug

app/Makefile.version

@@ -3,4 +3,4 @@ APPVERSION_M=4
 # This is the minor version of this release
 APPVERSION_N=0
 # This is the patch version of this release
-APPVERSION_P=9
+APPVERSION_P=10

app/rust/src/constants.rs

@@ -13,18 +13,6 @@
 *  See the License for the specific language governing permissions and
 *  limitations under the License.
 ********************************************************************************/
-
-pub const MAX_PAGES: usize = 9;
-// Configuration constants based on target device
-// means max number of lines per page
-pub const MAX_LINES: usize = 3;
-// means max characteres per line
-// this is a limit of the device
-// intended to control the already formatted
-// message response comming
-// from canisters
-pub const MAX_CHARS_PER_LINE: usize = 35;
-
 pub const BLS_PUBLIC_KEY_SIZE: usize = 96;
 pub const BLS_SIGNATURE_SIZE: usize = 48;
 pub const REPLY_PATH: &str = "reply";
@@ -33,8 +21,6 @@ pub const CANISTER_RANGES_PATH: &str = "canister_ranges";
 pub const CBOR_TAG: u64 = 55799;
 pub const BIG_NUM_TAG: u64 = 2;
 pub const CBOR_CERTIFICATE_TAG: u64 = CBOR_TAG;
-pub const CALL_REQUEST_TAG: u64 = CBOR_TAG;
-pub const CONSENT_MSG_REQUEST_TAG: u64 = CBOR_TAG;
 pub const CANISTER_RANGES_TAG: u64 = CBOR_TAG;
 pub const CANISTER_CALL_TAG: u64 = CBOR_TAG;
 
@@ -43,17 +29,12 @@ pub const PRINCIPAL_MAX_LEN: usize = 29;
 pub const SENDER_MAX_LEN: usize = 32; // Principal is bech32(29 bytes + 4 bytes for CRC)
 pub const ARG_HASH_LEN: usize = 32;
 pub const CANISTER_MAX_LEN: usize = 10;
-pub const REQUEST_MAX_LEN: usize = 10;
 pub const METHOD_MAX_LEN: usize = 20;
-pub const NONCE_MAX_LEN: usize = 32;
 pub const SECONDS_PER_MINUTE: u64 = 60;
 pub const NANOSECONDS_PER_SECOND: u64 = 1_000_000_000;
 // The max offset between the certificate.time and the call message request ingress_expiry
 // otherwise, call request must be considered invalid/outdated and not processed at all
 pub const MAX_CERT_INGRESS_OFFSET: u64 = 15 * SECONDS_PER_MINUTE * NANOSECONDS_PER_SECOND;
-// The official root key for consent message verification
-// including certificate, provided by the ICP team
-pub const CANISTER_ROOT_KEY: &str = "814c0e6ec71fab583b08bd81373c255c3c371b2e84863c98a4f1e08b74235d14fb5d9c0cd546d9685f913a0c0b2cc5341583bf4b4392e467db96d65b9bb4cb717112f8472e0d5a4d14505ffd7484b01291091c5f87b98883463f98091a0baaae";
 
 // Provided in testing data
 // indicating sender in there is the default one, whose value is 0x04

app/rust/src/ffi/c_api.rs

@@ -1,4 +1,4 @@
-use crate::{constants::PRINCIPAL_MAX_LEN, Principal};
+use crate::{constants::PRINCIPAL_MAX_LEN, error::ParserError, Principal};
 
 #[cfg(not(test))]
 extern "C" {
@@ -27,15 +27,29 @@ fn c_fill_principal(output: *mut u8, output_len: u16, response_len: *mut u16) ->
     }
 }
 
-// Get device principal, this is safe to use
-// because we ensure proper buffer sizes is passed to C
-pub fn device_principal() -> Principal {
+// Get device principal with proper error handling
+pub fn device_principal() -> Result<Principal, ParserError> {
     let mut data = [0u8; PRINCIPAL_MAX_LEN];
-    let mut response_len = 0;
+    let mut response_len = 0u16;
 
-    unsafe {
-        c_fill_principal(data.as_mut_ptr(), data.len() as _, &mut response_len);
+    let buffer_len = data.len();
+    if buffer_len > u16::MAX as usize {
+        return Err(ParserError::UnexpectedError);
     }
-
-    Principal::new(&data[..response_len as usize]).unwrap()
+    let rc = unsafe { c_fill_principal(data.as_mut_ptr(), buffer_len as u16, &mut response_len) };
+    
+    // Check return code for success
+    if rc != 0 {
+        return Err(ParserError::UnexpectedError);
+    }
+    
+    // Validate response_len is within valid range (1..=PRINCIPAL_MAX_LEN)
+    let response_len_usize = response_len as usize;
+    if response_len_usize == 0 || response_len_usize > PRINCIPAL_MAX_LEN {
+        return Err(ParserError::UnexpectedBufferEnd);
+    }
+    
+    // Use Principal::new and propagate any error
+    Principal::new(&data[..response_len_usize])
+        .map_err(|_| ParserError::UnexpectedValue)
 }

app/rust/src/ffi/call_request.rs

@@ -22,7 +22,7 @@ use crate::{
 use core::mem::{size_of, MaybeUninit};
 use sha2::{Digest, Sha256};
 
-use super::resources::MEMORY_CALL_REQUEST;
+use super::resources::get_call_request_memory;
 
 #[repr(C)]
 #[derive(PartialEq, Default)]
@@ -166,12 +166,23 @@ fn fill_request(request: &CallRequest<'_>) -> Result<(), ParserError> {
         // Update our consent request
         call_request.fill_from(request)?;
 
-        MEMORY_CALL_REQUEST
-            .write(0, &serialized)
+        super::resources::write_call_request(&serialized)
             .map_err(|_| ParserError::UnexpectedError)?;
 
-        let consent2 = CanisterCallT::from_bytes(&**MEMORY_CALL_REQUEST)?;
-        consent2.validate()?;
+        // Verify the write succeeded by reading back and validating
+        // Use a safe copy to avoid potential alignment issues
+        let stored_memory = super::resources::get_call_request_memory();
+        if stored_memory.len() != core::mem::size_of::<CanisterCallT>() {
+            return Err(ParserError::UnexpectedBufferEnd);
+        }
+        
+        let mut stored_call = CanisterCallT::default();
+        core::ptr::copy_nonoverlapping(
+            stored_memory.as_ptr(),
+            &mut stored_call as *mut CanisterCallT as *mut u8,
+            core::mem::size_of::<CanisterCallT>(),
+        );
+        stored_call.validate()?;
     }
 
     Ok(())
@@ -181,7 +192,7 @@ fn fill_request(request: &CallRequest<'_>) -> Result<(), ParserError> {
 pub unsafe extern "C" fn rs_get_signing_hash(data: *mut [u8; 32]) {
     let hash = unsafe { &mut *data };
 
-    let Ok(call) = CanisterCallT::from_bytes(&**MEMORY_CALL_REQUEST) else {
+    let Ok(call) = CanisterCallT::from_bytes(get_call_request_memory()) else {
         return;
     };
 

app/rust/src/ffi/consent_request.rs

@@ -25,7 +25,7 @@ use crate::{
 
 use core::mem::{size_of, MaybeUninit};
 
-use super::resources::MEMORY_CONSENT_REQUEST;
+use super::resources::get_consent_request_memory;
 
 #[repr(C)]
 #[derive(PartialEq, Default)]
@@ -188,19 +188,18 @@ fn fill_request(request: &ConsentMsgRequest<'_>) -> Result<(), ParserError> {
     let mut serialized = [0; core::mem::size_of::<ConsentRequestT>()];
     unsafe {
         {
-            // Lets transmute the array in order to re-use serialized memory
+            // Lets transmute the array in order to reuse serialized memory
             // saving a bunch of stack
             let consent_request = &mut *(serialized.as_mut_ptr() as *mut ConsentRequestT);
 
             // Update our consent request
             consent_request.fill_from(request)?;
 
-            MEMORY_CONSENT_REQUEST
-                .write(0, &serialized)
+            super::resources::write_consent_request(&serialized)
                 .map_err(|_| ParserError::UnexpectedError)?;
         }
 
-        let consent2 = ConsentRequestT::from_bytes(&**MEMORY_CONSENT_REQUEST)?;
+        let consent2 = ConsentRequestT::from_bytes(get_consent_request_memory())?;
         consent2.validate()?;
     }
 

app/rust/src/ffi/resources.rs

@@ -1,10 +1,13 @@
+#![allow(static_mut_refs)]
+
+use core::ptr::{addr_of_mut, addr_of};
 use bolos::{lazy_static, pic::PIC};
 
 use crate::{consent_message::msg_info::ConsentInfo, Certificate};
 
 use super::{call_request::CanisterCallT, consent_request::ConsentRequestT};
 
-// NOTE: The call_request anc consent_request were initially defined as Optionals, identical to the
+// NOTE: The call_request and consent_request were initially defined as Optionals, identical to the
 // certificate. But this approach consumes stacks, so we move it to flash, saving us 244 bytes of
 // stack, unfortunately, this did not help.
 // it is better to keep them as optionals, but we can change it back overtime.
@@ -20,11 +23,79 @@ pub static mut CERTIFICATE: Option<Certificate<'static>> = None;
 #[lazy_static]
 pub static mut UI: Option<ConsentInfo<'static>> = None;
 
+// Safe wrapper functions for static access
+pub(crate) unsafe fn clear_consent_request() {
+    let ptr = addr_of_mut!(MEMORY_CONSENT_REQUEST);
+    _ = (*ptr).write(0, &[0; core::mem::size_of::<ConsentRequestT>()]);
+}
+
+pub(crate) unsafe fn clear_call_request() {
+    let ptr = addr_of_mut!(MEMORY_CALL_REQUEST);
+    _ = (*ptr).write(0, &[0; core::mem::size_of::<CanisterCallT>()]);
+}
+
+pub(crate) unsafe fn write_call_request(data: &[u8]) -> Result<(), ()> {
+    let ptr = addr_of_mut!(MEMORY_CALL_REQUEST);
+    (*ptr).write(0, data).map_err(|_| ())
+}
+
+pub(crate) unsafe fn get_call_request_memory(
+) -> &'static [u8; core::mem::size_of::<CanisterCallT>()] {
+    let ptr = addr_of!(MEMORY_CALL_REQUEST);
+    &*ptr
+}
+
+pub(crate) unsafe fn write_consent_request(data: &[u8]) -> Result<(), ()> {
+    let ptr = addr_of_mut!(MEMORY_CONSENT_REQUEST);
+    (*ptr).write(0, data).map_err(|_| ())
+}
+
+pub(crate) unsafe fn get_consent_request_memory(
+) -> &'static [u8; core::mem::size_of::<ConsentRequestT>()] {
+    let ptr = addr_of!(MEMORY_CONSENT_REQUEST);
+    &*ptr
+}
+
+pub(crate) unsafe fn get_ui() -> Option<&'static ConsentInfo<'static>> {
+    let ptr = addr_of!(UI);
+    (*ptr).as_ref()
+}
+
+pub(crate) unsafe fn set_ui(ui: ConsentInfo<'static>) {
+    let ptr = addr_of_mut!(UI);
+    (*ptr).replace(ui);
+}
+
+pub(crate) unsafe fn take_ui() -> Option<ConsentInfo<'static>> {
+    let ptr = addr_of_mut!(UI);
+    (*ptr).take()
+}
+
+pub(crate) unsafe fn set_certificate(cert: Certificate<'static>) {
+    let ptr = addr_of_mut!(CERTIFICATE);
+    (*ptr).replace(cert);
+}
+
+pub(crate) unsafe fn take_certificate() -> Option<Certificate<'static>> {
+    let ptr = addr_of_mut!(CERTIFICATE);
+    (*ptr).take()
+}
+
+pub(crate) unsafe fn certificate_is_some() -> bool {
+    let ptr = addr_of!(CERTIFICATE);
+    (*ptr).is_some()
+}
+
+pub(crate) unsafe fn ui_is_some() -> bool {
+    let ptr = addr_of!(UI);
+    (*ptr).is_some()
+}
+
 #[no_mangle]
 pub unsafe extern "C" fn rs_clear_resources() {
     // zeroize data
-    _ = MEMORY_CONSENT_REQUEST.write(0, &[0; core::mem::size_of::<ConsentRequestT>()]);
-    _ = MEMORY_CALL_REQUEST.write(0, &[0; core::mem::size_of::<CanisterCallT>()]);
-    CERTIFICATE.take();
-    UI.take();
+    clear_consent_request();
+    clear_call_request();
+    take_certificate();
+    take_ui();
 }

app/rust/src/ffi/ui.rs

@@ -16,16 +16,16 @@
 
 use crate::{error::ParserError, DisplayableItem};
 
-use super::resources::UI;
+use super::resources::{get_ui, ui_is_some};
 
 #[no_mangle]
 pub unsafe extern "C" fn rs_getNumItems(num_items: *mut u8) -> u32 {
-    if num_items.is_null() || !UI.is_some() {
+    if num_items.is_null() || !ui_is_some() {
         return ParserError::ContextMismatch as u32;
     }
 
     // Safe to unwrap due to previous check
-    let ui = UI.as_ref().unwrap();
+    let ui = get_ui().unwrap();
 
     let Ok(num) = ui.num_items() else {
         return ParserError::NoData as _;
@@ -51,12 +51,12 @@ pub unsafe extern "C" fn rs_getItem(
     let key = core::slice::from_raw_parts_mut(out_key as *mut u8, key_len as usize);
     let value = core::slice::from_raw_parts_mut(out_value as *mut u8, out_len as usize);
 
-    if !UI.is_some() {
+    if !ui_is_some() {
         return ParserError::ContextMismatch as _;
     }
 
     // Safe to unwrap due to previous check
-    let ui = UI.as_ref().unwrap();
+    let ui = get_ui().unwrap();
 
     match ui.render_item(display_idx, key, value, page_idx) {
         Ok(page) => {
@@ -77,12 +77,12 @@ pub unsafe extern "C" fn rs_get_intent(out_intent: *mut i8, intent_len: u16) ->
     let out_slice = core::slice::from_raw_parts_mut(out_intent as *mut u8, intent_len as usize);
     out_slice[0] = 0;
 
-    if !UI.is_some() {
+    if !ui_is_some() {
         return ParserError::ContextMismatch as u32;
     }
 
     // Safe to unwrap due to previous check
-    let ui = UI.as_ref().unwrap();
+    let ui = get_ui().unwrap();
 
     // Access the intent using the public method
     if let Some(intent) = ui.message.get_intent() {