safety: introduce pointer types and their restrictions #208

muzarski · 2024-11-26T15:47:47Z

This is a follow-up to: #207.

It defines a CassPtr type, wrapper over Option<NonNull<T>>, generic over the pointer Properties. Two properties we do care about:

ownership - Exclusive (BoxFFI), Shared (ArcFFI), Borrowed (RefFFI)
mutability - Const or Mut

It gives us even more guarantees about the pointers. Specifically, it gives us the guarantee about the origin of the pointer - they can only be constructed via Box/Ref/ArcFFI APIs. We lacked this guarantee before this PR.

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
PR description sums up the changes and reasons why they should be introduced.
I have implemented Rust unit tests for the features/changes introduced.
~~[ ] I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.~~
~~[ ] I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.~~

wprzytula

The approach is very impressive.
I couldn't check all the changes, there were too much of them; I just peeked through them. I trust your meticulousness :)

scylla-rust-wrapper/src/collection.rs

scylla-rust-wrapper/src/metadata.rs

scylla-rust-wrapper/src/argconv.rs

muzarski · 2024-12-02T15:05:58Z

Rebased on master

muzarski · 2024-12-02T19:32:07Z

v1.1: Addressed @wprzytula comments

wprzytula · 2024-12-04T12:09:41Z

Waiting with review for a rebase on #207.

scylla-rust-wrapper/src/argconv.rs

muzarski · 2024-12-04T12:38:42Z

Waiting with review for a rebase on #207.

Everything is up to date

scylla-rust-wrapper/src/argconv.rs

muzarski · 2024-12-04T18:09:34Z

v2: Improved pointer API based on @wprzytula suggestions. Thank you!

removed Copy and Clone implementations for CassPtr (currently, it's only defined for test purposes. I need to think how to address it some other way)
Left CassPtr::into_ref() as is. As discussed in the comment, it is required to "erase" the lifetime of the returned reference. This method consumes the pointer.
Introduced CassPtr::as_ref() method. It accepts a reference to the pointer. The lifetime of reference is inherited from pointer's lifetime.
Changed CassExclusiveMutPtr::into_mut_ref() to [...]::as_mut_ref(). It accepts &mut CassExclusiveMutPtr. The lifetime of returned mutable reference is inherited from mutable borrow of the pointer. This is really cool, because this ensures aliasing ^ mutability at borrow-checker level.
CassPtr::is_null() now borrows the pointer, instead of consuming it
CassFuture::set_callback(), and CassFutureCallback now expect a raw pointer. Safe CassPtr::as_raw() method is introduced for this use-case

I still need to think how to address the issue in tests. It's not possible to reuse the pointer, which is not Copy, while passing it to multiple API functions that consume the pointer.

wprzytula · 2024-12-04T20:30:57Z

I still need to think how to address the issue in tests. It's not possible to reuse the pointer, which is not Copy, while passing it to multiple API functions that consume the pointer.

You could have an unsafe clone() method for tests only, then this would not break soundness guarantees of safe code.

muzarski · 2024-12-04T22:43:47Z

I still need to think how to address the issue in tests. It's not possible to reuse the pointer, which is not Copy, while passing it to multiple API functions that consume the pointer.

You could have an unsafe clone() method for tests only, then this would not break soundness guarantees of safe code.

Applied the suggestion. At first, I did not understand how is this different from implementing Copy and Clone for test only. But this part: "this would not break soundness guarantees of safe code" convinced me.

scylla-rust-wrapper/src/argconv.rs

scylla-rust-wrapper/src/batch.rs

muzarski · 2024-12-09T12:46:02Z

Rebased on master

muzarski · 2024-12-09T12:54:07Z

v2.1: improved docstrings as suggested by @wprzytula

Lorak-mmk · 2024-12-11T15:08:49Z

I've read the first few commits, and it talks about cass_data_type_new_from_existing expecting a Arc-allocated CassDataType, but I don't see why.
For context, the code of this function:

#[no_mangle]
pub unsafe extern "C" fn cass_data_type_new_from_existing(
    data_type: *const CassDataType,
) -> *const CassDataType {
    let data_type = ptr_to_ref(data_type);
    Arc::into_raw(Arc::new(data_type.clone()))
}

As far as I can tell it copies the whole object, not Arc, and does not assume it is Arc-allocated.

muzarski · 2024-12-11T15:30:29Z

I've read the first few commits, and it talks about cass_data_type_new_from_existing expecting a Arc-allocated CassDataType, but I don't see why. For context, the code of this function:
#[no_mangle]
pub unsafe extern "C" fn cass_data_type_new_from_existing(
    data_type: *const CassDataType,
) -> *const CassDataType {
    let data_type = ptr_to_ref(data_type);
    Arc::into_raw(Arc::new(data_type.clone()))
}
As far as I can tell it copies the whole object, not Arc, and does not assume it is Arc-allocated.

Sorry for the confusion. I remember that I made a mistake in some commit message, but forgot to fix it. I meant cass_data_type_add_sub_type function:

#[no_mangle]
pub unsafe extern "C" fn cass_data_type_add_sub_type(
    data_type: CassSharedPtr<CassDataType>,
    sub_data_type: CassSharedPtr<CassDataType>,
) -> CassError {
    let data_type = ArcFFI::as_ref(&data_type).unwrap();
    match data_type
        .get_mut_unchecked()
        .add_sub_data_type(ArcFFI::cloned_from_ptr(sub_data_type).unwrap())
    {
        Ok(()) => CassError::CASS_OK,
        Err(e) => e,
    }
}

muzarski · 2024-12-11T15:32:22Z

Pushed fixed commit message (collection: allocate static empty collections).

Lorak-mmk · 2024-12-11T15:46:47Z

The same mistake is present in metadata: store CassDataType behind an Arc commit message.

muzarski · 2024-12-12T02:04:08Z

Rebased on master

Lorak-mmk

I did not review the whole thing, because I spotted some important issues in argconv.rs. In general, please describe with more detail in precision what guarantees / requirements the types / methods require and provide. Without it it is a bit difficult to reason about the code.

Lorak-mmk · 2024-12-13T18:52:28Z

scylla-rust-wrapper/src/metadata.rs

    view_meta: *const CassMaterializedViewMeta,
 ) -> *const CassTableMeta {
    let view_meta = RefFFI::as_ref(view_meta);
-    view_meta.base_table.as_ptr()
+
+    match view_meta.base_table.upgrade() {
+        Some(arc) => RefFFI::as_ptr(&arc),
+        None => std::ptr::null(),
+    }


Is it possible for this upgrade to fail? Or does it indicate a bug in the driver?
If it is possible, can we have a test for that?

Lorak-mmk · 2024-12-13T19:21:12Z

scylla-rust-wrapper/src/future.rs

 impl BoundCallback {
-    fn invoke(self, fut: &CassFuture) {
+    fn invoke(self, fut_ptr: *const CassFuture) {
        unsafe {
-            self.cb.unwrap()(fut as *const CassFuture, self.data);
+            self.cb.unwrap()(fut_ptr, self.data);
        }
    }
 }


Shouldn't this invoke method be unsafe? It does have a requirement for its argument to be a correct (alingment, lifetime etc) pointer.

Lorak-mmk · 2024-12-13T19:23:35Z

scylla-rust-wrapper/src/future.rs

+    pub fn set_callback(
+        &self,
+        self_ptr: *const CassFuture,
+        cb: CassFutureCallback,
+        data: *mut c_void,
+    ) -> CassError {
        let mut lock = self.state.lock().unwrap();
        if lock.callback.is_some() {
            // Another callback has been already set


Here self_ptr should refer to the same CassFuture as self, right?
Can you instead make the receiver of this function an arc? self: &Arc<CassFuture>

scylla-rust-wrapper/src/argconv.rs

muzarski · 2024-12-17T20:20:10Z

v3:

New `Origin` trait and modified `Ownership` trait

Introduced new Origin trait which represents an origin of the pointer, and three implementors:

FromBox - box allocated, exclusive (potentially mutable) pointers
FromArc - arc-allocated, shared (either owned, or borrowed) const pointers
FromRef - pointers created from a valid rust reference, always borrowed and const

Modified implementors of Ownership trait. Now we distinguish two ownership kinds:

Owned - owned pointer. Data needs to be freed
Borrowed - borrowed pointers. Conversions to reference/Arcs from such pointers are unsafe - caller needs to ensure that owner of the data is alive

Additional commit

Moved the changes that introduce CassPtr (and traits) to a separate commit. I hope this aids readability and reduces the noise during review.

Thoroughly documented each type/trait and its methods. Explained what each type guarantees (and what it does not guarantee). Justified the choice of the methods being safe/unsafe.

Other parts of the code make an assumption, that the pointer representing `CassDataType` was obtained from an Arc allocation. Take for example `cass_data_type_add_sub_type` - it clones an Arc. This is a bug, that was fortunately detected by applying more restrictions on the pointer types (introduced later in this PR).

The same bug as for collection types.

Again, if someone called `cass_data_type_add_sub_type` with a data type obtained from `cass_column_meta_data_type`, it would not be a pointer from an Arc allocation.

Weak::as_ptr() can return an invalid pointer. It can be even dangling (non-null). It's safer to try to upgrade to an Arc. If upgrade was successful, make use of RefFFI API to return a valid pointer. Otherwise, return non-dangling null pointer.

Before this PR, the pointer was obtained from a valid reference &CassFuture, which is totally fine. However, I want to reduce the ways one can obtain such pointer. For ArcFFI (shared pointers), I want them to be obtainable only in two ways: - `ArcFFI::as_ptr()` which accepts an &Arc - from the user, as a function parameter This way, we are guaranteed that the pointer comes from a valid Arc allocation (unless user provided pointer to some garbage, but there is no much we can do about it). If we assume that user provides a pointer returned from some prior call to API, we are guaranteed that it is valid, and comes from an Arc allocation (or is null). I don't want to allow ArcFFI api to create a pointer from a refernce, to prevent creating a pointer, from example from stack allocated object: ``` let future = CassFuture { ... }; let future_ptr = ArcFFI::as_ptr(&future); ``` This commit may not make much sense now, but all should be clear once I introduce traits restricting the pointer types later in this PR.

cpp-driver does not increase the reference count. The lifetime of the iterator is bound to the lifetime of result.

It's more readable (and safe) to have an explicit lifetime instead of lifetime-erased references.

cpp-driver assumes that session object can be prematurely dropped. This means, that we should increase the reference count in functions where we pass the session to an async block. This will prevent UAF. There actually is a test case for this (AsyncTests::Close). However, we cannot enable it yet, since it expects that prematurely dropped session awaits all async tasks and before closing.

It's required to enable the doctests for the crate. I also marked the code snippets from documentation in binding.rs with `text`. They should not be run as doc tests.

This commit introduces a `CassPtr` type, generic over pointer `Properties`. It allows specific pointer-to-reference conversions based on the guarantees provided by the pointer type.

Existing `Ref/Box/Arc`FFIs are adjusted, so they now allow interaction with new pointer type. You can say, that for a user of `argconv` API, new type is opaque. The only way to operate on it is to use the corresponding ffi API.

Disallow implementing two different APIs for specific type.

muzarski · 2025-03-10T06:43:47Z

Rebased on master

muzarski marked this pull request as draft November 26, 2024 15:47

muzarski force-pushed the pointer-kinds branch 2 times, most recently from f433610 to 9dadad8 Compare November 26, 2024 17:35

muzarski self-assigned this Nov 26, 2024

muzarski force-pushed the pointer-kinds branch from 9dadad8 to 7b8bb5a Compare November 29, 2024 12:44

muzarski requested review from Lorak-mmk and wprzytula November 29, 2024 12:47

muzarski marked this pull request as ready for review November 29, 2024 12:47

wprzytula requested changes Dec 1, 2024

View reviewed changes

muzarski force-pushed the pointer-kinds branch from 7b8bb5a to e8d2d93 Compare December 2, 2024 15:05

muzarski force-pushed the pointer-kinds branch 2 times, most recently from 98fbac9 to aa90f5a Compare December 2, 2024 19:30

muzarski requested a review from wprzytula December 2, 2024 19:32

wprzytula reviewed Dec 4, 2024

View reviewed changes

scylla-rust-wrapper/src/argconv.rs Outdated Show resolved Hide resolved

wprzytula requested changes Dec 4, 2024

View reviewed changes

scylla-rust-wrapper/src/argconv.rs Outdated Show resolved Hide resolved

scylla-rust-wrapper/src/argconv.rs Outdated Show resolved Hide resolved

muzarski force-pushed the pointer-kinds branch from aa90f5a to 6c2a061 Compare December 4, 2024 18:00

muzarski requested a review from wprzytula December 4, 2024 18:37

muzarski force-pushed the pointer-kinds branch from 6c2a061 to 3686e0d Compare December 4, 2024 22:39

wprzytula approved these changes Dec 6, 2024

View reviewed changes

scylla-rust-wrapper/src/argconv.rs Outdated Show resolved Hide resolved

scylla-rust-wrapper/src/argconv.rs Outdated Show resolved Hide resolved

scylla-rust-wrapper/src/batch.rs Outdated Show resolved Hide resolved

muzarski force-pushed the pointer-kinds branch from 3686e0d to f341734 Compare December 9, 2024 12:45

muzarski force-pushed the pointer-kinds branch from f341734 to 8dfff0a Compare December 9, 2024 12:53

muzarski force-pushed the pointer-kinds branch from 8dfff0a to d4d7b1e Compare December 11, 2024 15:31

muzarski force-pushed the pointer-kinds branch 2 times, most recently from ec6be4b to cf2dcff Compare December 12, 2024 02:01

Lorak-mmk requested changes Dec 16, 2024

View reviewed changes

muzarski force-pushed the pointer-kinds branch from cf2dcff to 66b329e Compare December 17, 2024 20:13

muzarski requested a review from Lorak-mmk December 17, 2024 20:21

muzarski force-pushed the pointer-kinds branch 3 times, most recently from bc520d3 to 69d46b3 Compare December 19, 2024 16:06

muzarski added 2 commits March 10, 2025 07:34

tuple: allocate empty tuple type

0910dbd

The same bug as for collection types.

muzarski force-pushed the pointer-kinds branch from 69d46b3 to 73f3f68 Compare March 10, 2025 06:40

muzarski added 10 commits March 10, 2025 07:42

metadata: store CassDataType behind an Arc

31c1530

Again, if someone called `cass_data_type_add_sub_type` with a data type obtained from `cass_column_meta_data_type`, it would not be a pointer from an Arc allocation.

iter: hold reference to CassResult in CassResultIterator

6faaee2

cpp-driver does not increase the reference count. The lifetime of the iterator is bound to the lifetime of result.

iter: parameterize CassIterator with lifetime

389b5d1

It's more readable (and safe) to have an explicit lifetime instead of lifetime-erased references.

cargo: add "lib" to crate-type list

b238654

It's required to enable the doctests for the crate. I also marked the code snippets from documentation in binding.rs with `text`. They should not be run as doc tests.

argconv: introduce CassPtr

a4ee131

This commit introduces a `CassPtr` type, generic over pointer `Properties`. It allows specific pointer-to-reference conversions based on the guarantees provided by the pointer type.

argconv: adjust FFI apis to new pointer type

88e6c1a

Existing `Ref/Box/Arc`FFIs are adjusted, so they now allow interaction with new pointer type. You can say, that for a user of `argconv` API, new type is opaque. The only way to operate on it is to use the corresponding ffi API.

argconv: make FFI implementation mutually exclusive

5c0af8e

Disallow implementing two different APIs for specific type.

muzarski force-pushed the pointer-kinds branch from 73f3f68 to 5c0af8e Compare March 10, 2025 06:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

safety: introduce pointer types and their restrictions #208

safety: introduce pointer types and their restrictions #208

muzarski commented Nov 26, 2024 •

edited

Loading

wprzytula left a comment

muzarski commented Dec 2, 2024

muzarski commented Dec 2, 2024

wprzytula commented Dec 4, 2024

muzarski commented Dec 4, 2024

muzarski commented Dec 4, 2024 •

edited

Loading

wprzytula commented Dec 4, 2024

muzarski commented Dec 4, 2024

muzarski commented Dec 9, 2024

muzarski commented Dec 9, 2024

Lorak-mmk commented Dec 11, 2024

muzarski commented Dec 11, 2024

muzarski commented Dec 11, 2024

Lorak-mmk commented Dec 11, 2024

muzarski commented Dec 12, 2024

Lorak-mmk left a comment

Lorak-mmk Dec 13, 2024

Lorak-mmk Dec 13, 2024

Lorak-mmk Dec 13, 2024

muzarski commented Dec 17, 2024

muzarski commented Mar 10, 2025

safety: introduce pointer types and their restrictions #208

Are you sure you want to change the base?

safety: introduce pointer types and their restrictions #208

Conversation

muzarski commented Nov 26, 2024 • edited Loading

Pre-review checklist

wprzytula left a comment

Choose a reason for hiding this comment

muzarski commented Dec 2, 2024

muzarski commented Dec 2, 2024

wprzytula commented Dec 4, 2024

muzarski commented Dec 4, 2024

muzarski commented Dec 4, 2024 • edited Loading

wprzytula commented Dec 4, 2024

muzarski commented Dec 4, 2024

muzarski commented Dec 9, 2024

muzarski commented Dec 9, 2024

Lorak-mmk commented Dec 11, 2024

muzarski commented Dec 11, 2024

muzarski commented Dec 11, 2024

Lorak-mmk commented Dec 11, 2024

muzarski commented Dec 12, 2024

Lorak-mmk left a comment

Choose a reason for hiding this comment

Lorak-mmk Dec 13, 2024

Choose a reason for hiding this comment

Lorak-mmk Dec 13, 2024

Choose a reason for hiding this comment

Lorak-mmk Dec 13, 2024

Choose a reason for hiding this comment

muzarski commented Dec 17, 2024

New Origin trait and modified Ownership trait

Additional commit

muzarski commented Mar 10, 2025

muzarski commented Nov 26, 2024 •

edited

Loading

muzarski commented Dec 4, 2024 •

edited

Loading

New `Origin` trait and modified `Ownership` trait