From 9134e98dc066034d6fe9b163816f1258b0fa423b Mon Sep 17 00:00:00 2001 From: Colton Hurst Date: Tue, 20 Aug 2024 10:38:25 -0400 Subject: [PATCH] [SM-1413] Resolve Concurrency Issues in C FFI (#981) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 🎟ī¸ Tracking https://bitwarden.atlassian.net/browse/SM-1413 ## 📔 Objective ### The Issue Through the C FFI, we were running into concurrency issues with multiple Tokio runtimes. A draft PR showing the concurrency issue with instructions can be found [here](https://github.com/bitwarden/sdk/pull/955). ### The Fix This PR fixes this issue for `bitwarden-c` and `bitwarden-py`, by preserving the runtime. Thanks @dani-garcia for the fix and working together on this one! ### Extra This also refactors the `AccessTokenLogin` type as a follow up for testing purposes for Go as initiated by [this](https://github.com/bitwarden/sdk/pull/953) PR. ## ⏰ Reminders before review - Contributor guidelines followed - All formatters and local linters executed and passed - Written new unit and / or integration tests where applicable - Protected functional changes with optionality (feature flags) - Used internationalization (i18n) for all UI strings - CI builds passed - Communicated to DevOps any deployment requirements - Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team ## đŸĻŽ Reviewer guidelines - 👍 (`:+1:`) or similar for great changes - 📝 (`:memo:`) or ℹī¸ (`:information_source:`) for notes or general info - ❓ (`:question:`) for questions - 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion - 🎨 (`:art:`) for suggestions / improvements - ❌ (`:x:`) or ⚠ī¸ (`:warning:`) for more significant problems or concerns needing attention - 🌱 (`:seedling:`) or â™ģī¸ (`:recycle:`) for future improvements or indications of technical debt - ⛏ (`:pick:`) for minor or nitpick changes --- crates/bitwarden-c/src/c.rs | 37 ++++++++++++++++++++++--------- crates/bitwarden-py/src/client.rs | 16 ++++++------- languages/go/bitwarden_client.go | 2 +- 3 files changed, 35 insertions(+), 20 deletions(-) diff --git a/crates/bitwarden-c/src/c.rs b/crates/bitwarden-c/src/c.rs index 32abe3dc0..bd10f7d88 100644 --- a/crates/bitwarden-c/src/c.rs +++ b/crates/bitwarden-c/src/c.rs @@ -4,17 +4,24 @@ use bitwarden_json::client::Client; use crate::{box_ptr, ffi_ref}; +#[repr(C)] +pub struct CClient { + /// Associates the tokio runtime to the `Client`, ensuring the runtime has the same lifecycle + /// as the `Client`. + runtime: tokio::runtime::Runtime, + client: Client, +} + #[no_mangle] -#[tokio::main] -pub async extern "C" fn run_command( - c_str_ptr: *const c_char, - client_ptr: *const Client, -) -> *mut c_char { +pub extern "C" fn run_command(c_str_ptr: *const c_char, client_ptr: *const CClient) -> *mut c_char { let client = unsafe { ffi_ref!(client_ptr) }; let input_str = str::from_utf8(unsafe { CStr::from_ptr(c_str_ptr) }.to_bytes()) .expect("Input should be a valid string"); - let result = client.run_command(input_str).await; + let result = client + .runtime + .block_on(client.client.run_command(input_str)); + match std::ffi::CString::new(result) { Ok(cstr) => cstr.into_raw(), Err(_) => panic!("failed to return command result: null encountered"), @@ -23,17 +30,25 @@ pub async extern "C" fn run_command( // Init client, potential leak! You need to call free_mem after this! #[no_mangle] -pub extern "C" fn init(c_str_ptr: *const c_char) -> *mut Client { +pub extern "C" fn init(c_str_ptr: *const c_char) -> *mut CClient { // This will only fail if another logger was already initialized, so we can ignore the result let _ = env_logger::try_init(); - if c_str_ptr.is_null() { - box_ptr!(Client::new(None)) + + let runtime = tokio::runtime::Builder::new_multi_thread() + .enable_all() + .build() + .expect("Failed to build tokio runtime"); + + let client = if c_str_ptr.is_null() { + Client::new(None) } else { let input_string = str::from_utf8(unsafe { CStr::from_ptr(c_str_ptr) }.to_bytes()) .expect("Input should be a valid string") .to_owned(); - box_ptr!(Client::new(Some(input_string))) - } + Client::new(Some(input_string)) + }; + + box_ptr!(CClient { runtime, client }) } // Free mem diff --git a/crates/bitwarden-py/src/client.rs b/crates/bitwarden-py/src/client.rs index 510de1db4..9c12a624d 100644 --- a/crates/bitwarden-py/src/client.rs +++ b/crates/bitwarden-py/src/client.rs @@ -2,7 +2,7 @@ use bitwarden_json::client::Client as JsonClient; use pyo3::prelude::*; #[pyclass] -pub struct BitwardenClient(JsonClient); +pub struct BitwardenClient(tokio::runtime::Runtime, JsonClient); #[pymethods] impl BitwardenClient { @@ -13,16 +13,16 @@ impl BitwardenClient { // result let _ = pyo3_log::try_init(); - Self(JsonClient::new(settings_string)) + let runtime = tokio::runtime::Builder::new_multi_thread() + .enable_all() + .build() + .expect("Failed to build tokio runtime"); + + Self(runtime, JsonClient::new(settings_string)) } #[pyo3(text_signature = "($self, command_input)")] fn run_command(&self, command_input: String) -> String { - run_command(&self.0, &command_input) + self.0.block_on(self.1.run_command(&command_input)) } } - -#[tokio::main] -async fn run_command(client: &JsonClient, input_str: &str) -> String { - client.run_command(input_str).await -} diff --git a/languages/go/bitwarden_client.go b/languages/go/bitwarden_client.go index c6c9e09b4..5e951ecd1 100644 --- a/languages/go/bitwarden_client.go +++ b/languages/go/bitwarden_client.go @@ -54,7 +54,7 @@ func NewBitwardenClient(apiURL *string, identityURL *string) (BitwardenClientInt func (c *BitwardenClient) AccessTokenLogin(accessToken string, stateFile *string) error { req := AccessTokenLoginRequest{AccessToken: accessToken, StateFile: stateFile} - command := Command{AccessTokenLogin: &req} + command := Command{LoginAccessToken: &req} responseStr, err := c.commandRunner.RunCommand(command) if err != nil {