Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix alignment #95

Merged
merged 3 commits into from Jun 1, 2023
Merged

Fix alignment #95

merged 3 commits into from Jun 1, 2023

Conversation

Wonshtrum
Copy link
Contributor

Fdb Arenas allocate structs not aligned, even though fdb_c.h explicitly specifies a 4bytes alignment for most of them (the others using their default alignment, 8bytes in general):

#pragma pack(push, 4)
typedef struct key {
const uint8_t* key;
int key_length;
} FDBKey;

These alignment constraints are translated in Rust in bindings.rs by bindgen and exported by foundationdb_sys:

bindings
.write_to_file(out_path.join("bindings.rs"))

#[repr(C, packed(4))]
#[derive(Debug, Copy, Clone)]
pub struct key {
    pub key: *const u8,
    pub key_length: ::std::os::raw::c_int,
}
pub type FDBKey = key;

And inherited by their "local wrappers":

#[repr(transparent)]
/// An FdbKey, owned by a FoundationDB Future
pub struct FdbKey(fdb_sys::FDBKey);

Data is transferred between the fdbserver and foundationdb-rs via pointers which lead to this kind of code:

fn deref(&self) -> &Self::Target {
assert_eq_size!(FdbKey, fdb_sys::FDBKey);
assert_eq_align!(FdbKey, fdb_sys::FDBKey);
unsafe {
&*(std::slice::from_raw_parts(self.keys, self.len as usize)
as *const [fdb_sys::FDBKey] as *const [FdbKey])
}
}

self.keys is a C pointer that points to a FDBKey potentially misaligned in a Fdb Arena. In the newer versions of Rust from_raw_parts panics with debug_assertions on pointer misalignment. Even if we ignore this, the fact that misaligned structs are currently read correctly is probably due to x86 compliance but is still undefined behavior.

In this PR, "local wrappers" are annotated with packed instead of transparent:
https://github.com/Wonshtrum/foundationdb-rs/blob/c6b198ddef85ca77bab40ea11cda1c08c9d09e35/foundationdb/src/fdb_keys.rs#L157-L159

This explicitly tells Rust that we do not expect these structs to be aligned. This removes the panics on newer versions as these structs are 1byte align (which a pointer will always be) and furthermore, we do not have to rely on x86 to magically accept unaligned structs as misalignment will be explicitly taken care of at machine code generation.

These wrappers are used everywhere in favor of their raw counterparts from fdb_sys, and deref functions are rewritten using this form:
https://github.com/Wonshtrum/foundationdb-rs/blob/c6b198ddef85ca77bab40ea11cda1c08c9d09e35/foundationdb/src/fdb_keys.rs#L46-L50

note also the assert_eq_align!(FdbKey, u8);

PierreZ and others added 3 commits May 31, 2023 11:32
Packed structs are unaligned (or aligned to 1 byte).
This makes the code resilient to unaligned Fdb Arenas.
@PierreZ PierreZ self-requested a review May 31, 2023 11:26
@PierreZ PierreZ added bug Something isn't working correctness run hundred of iterations on the bindingTester during CI and removed correctness run hundred of iterations on the bindingTester during CI labels May 31, 2023
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 94.73% and project coverage change: -0.17 ⚠️

Comparison is base (e846355) 79.84% compared to head (c6b198d) 79.68%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #95      +/-   ##
==========================================
- Coverage   79.84%   79.68%   -0.17%     
==========================================
  Files          27       27              
  Lines        5181     5207      +26     
==========================================
+ Hits         4137     4149      +12     
- Misses       1044     1058      +14     
Impacted Files Coverage Δ
foundationdb/src/mapped_key_values.rs 45.04% <88.88%> (+1.00%) ⬆️
foundationdb/src/fdb_keys.rs 60.93% <100.00%> (+1.92%) ⬆️
foundationdb/src/future.rs 80.43% <100.00%> (+0.10%) ⬆️

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@PierreZ PierreZ merged commit 0007dd2 into foundationdb-rs:main Jun 1, 2023
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctness run hundred of iterations on the bindingTester during CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants