ivila opened a new issue, #153: URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/issues/153
## Description The parameter types of the invoke function is inappropriate. https://github.com/apache/incubator-teaclave-trustzone-sdk/blob/bc14fb63f65bfff77456eea1b9097c1619d30b04/optee-utee/src/extension.rs#L33-L56 First, **the parameter data should be declared as mut instead**, as when invoking plugin the content of the buffer will be changed: Secondly, I think the function should just like [`the read function of std::io::Read`](https://doc.rust-lang.org/std/io/trait.Read.html#tymethod.read), return the length rather than return a new constructed vector, returning a length notifies developers that **the output is relative to the limited buffer**, while returning a vector is not. Last, I think the function should add an parameter_size parameter as sometimes **the return content is bigger than the request content**, for example, fetch file content from plugin. in conclusion, I think the function should be: ```rust // previous: pub fn invoke(&mut self, command_id: u32, subcommand_id: u32, data: &[u8]) -> Result<Vec<u8>> { // now: set data as mut and rename it to share_buffer, add parameter_size and return length instead of new vector pub fn invoke(&mut self, command_id: u32, subcommand_id: u32, share_buffer: &mut [u8], parameter_size: usize) -> Result<usize> { // some codes } ``` ## Suggestion ### 1) change the parameters and return value of the function, but it is a breaking change ``` rust impl LoadablePlugin { pub fn new(uuid: &Uuid) -> Self { Self { uuid: uuid.to_owned() } } pub fn invoke(&mut self, command_id: u32, subcommand_id: u32, data: &mut [u8], parameter_size: usize) -> Result<usize> { let raw_uuid: Uuid = self.uuid; let mut outlen: usize = 0; match unsafe { raw::tee_invoke_supp_plugin( raw_uuid.as_raw_ptr(), command_id as u32, subcommand_id as u32, data.as_ptr() as _, parameter_size, &mut outlen as *mut usize, ) } { raw::TEE_SUCCESS => { assert!(outlen <= (data.len())); Ok(outlen) }, code => Err(Error::from_raw_error(code)), } } } ``` ### 2) mark the invoke method as deprecated, and introduce a new one ``` rust impl LoadablePlugin { pub fn new(uuid: &Uuid) -> Self { Self { uuid: uuid.to_owned() } } #[deprecated(note = "Please use the invoke2 function instead")] pub fn invoke(&mut self, command_id: u32, subcommand_id: u32, data: &[u8]) -> Result<Vec<u8>> { // old codes } pub fn invoke2(&mut self, command_id: u32, subcommand_id: u32, data: &mut [u8], parameter_size: usize) -> Result<usize> { let raw_uuid: Uuid = self.uuid; let mut outlen: usize = 0; match unsafe { raw::tee_invoke_supp_plugin( raw_uuid.as_raw_ptr(), command_id as u32, subcommand_id as u32, data.as_ptr() as _, parameter_size, &mut outlen as *mut usize, ) } { raw::TEE_SUCCESS => { assert!(outlen <= (data.len())); Ok(outlen) }, code => Err(Error::from_raw_error(code)), } } } ``` I would prefer the first one, as the codes are wrong here, we should stop people from compiling so that they will get noticed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@teaclave.apache.org.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@teaclave.apache.org For additional commands, e-mail: dev-h...@teaclave.apache.org
