Skip to content

Commit

Permalink
[SM-1413] Resolve Concurrency Issues in C FFI (#981)
Browse files Browse the repository at this point in the history
## 🎟️ 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](#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](#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

<!-- Suggested interactions but feel free to use (or not) as you desire!
-->

- 👍 (`:+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
  • Loading branch information
coltonhurst authored Aug 20, 2024
1 parent 5149ec3 commit 9134e98
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 20 deletions.
37 changes: 26 additions & 11 deletions crates/bitwarden-c/src/c.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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
Expand Down
16 changes: 8 additions & 8 deletions crates/bitwarden-py/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}
2 changes: 1 addition & 1 deletion languages/go/bitwarden_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 9134e98

Please sign in to comment.