Skip to content

safety: pointer related bugfixes #229

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

Merged
merged 5 commits into from
Mar 13, 2025
Merged

Conversation

muzarski
Copy link
Collaborator

@muzarski muzarski commented Mar 12, 2025

This PR introduces multiple fixes around pointer manipulation in the driver. These bugs were found thanks to the CassPtr which will be introduced in a follow-up 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.

@muzarski muzarski marked this pull request as draft March 12, 2025 11:41
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 (CassPtr which will be introduced in follow-up 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.
@muzarski muzarski requested review from wprzytula and Lorak-mmk March 12, 2025 11:55
@muzarski muzarski self-assigned this Mar 12, 2025
@muzarski muzarski added this to the 0.4 milestone Mar 12, 2025
@muzarski muzarski added the bug Something isn't working label Mar 12, 2025
@muzarski muzarski marked this pull request as ready for review March 12, 2025 11:56
Comment on lines 263 to 268
pub unsafe fn set_callback(
&self,
self_ptr: *mut CassFuture,
cb: CassFutureCallback,
data: *mut c_void,
) -> CassError {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ What do we achieve by passing *mut Self alongside &Self? Can't we just cast &Self to *mut Self in set_callback's body?

Copy link
Collaborator Author

@muzarski muzarski Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is explained in commit message. It's preparation for CassPtr introduction. This *mut CassFuture will be replaced by CassPtr.

I don't want expose an API to convert a reference to CassPtr via ArcFFI. I want to be sure that ArcFFI creates CassPtr which are backed by Arc allocation. We don't have such guarantee if we allow to create a pointer from just ref (&).

Here is a snippet from the follow-up PR. These are the only methods on ArcFFI that convert rust reference primitives to pointers:

pub trait ArcFFI: Sized {
    /// Creates a pointer from a valid reference to Arc-allocated data.
    /// Holder of the pointer borrows the pointee.
    fn as_ptr<'a>(self: &'a Arc<Self>) -> CassPtr<'a, Self, (Const,)> {
        #[allow(clippy::disallowed_methods)]
        let ptr = Arc::as_ptr(self);

        // SAFETY:
        // 1. validity guarantee - pointer is valid, since it's obtained from Arc allocation
        // 2. pointer's lifetime - pointer inherits the lifetime of provided Arc's borrow.
        //    What's important is that the returned pointer borrows the data, and is not the
        //    shared owner. Thus, user cannot call ArcFFI::free() on such pointer.
        // 3. mutability - we always create a `Const` pointer.
        unsafe { CassPtr::from_raw(ptr) }
    }

    /// Creates a pointer from a valid Arc allocation.
    fn into_ptr(self: Arc<Self>) -> CassPtr<'static, Self, (Const,)> {
        #[allow(clippy::disallowed_methods)]
        let ptr = Arc::into_raw(self);

        // SAFETY:
        // 1. validity guarantee - pointer is valid, since it's obtained from Arc allocation
        // 2. pointer's lifetime - returned pointer has a 'static lifetime. It is a shared
        //    owner of the pointee. User has to decrease the RC of the pointer (and potentially free the memory)
        //    via ArcFFI::free().
        // 3. mutability - we always create a `Const` pointer.
        unsafe { CassPtr::from_raw(ptr) }
    }
}

Notice that if we expose a method like fn as_ptr_from_ref(&self) -> CassPtr<'_, Self, (Const,)>, we lose the guarantee that this reference originates from some Arc allocation.

Why do we need such guarantee? An example:

struct Foo;
impl ArcFFI for Foo {}

let foo = Foo;
let ptr: CassPtr = ArcFFI::as_ptr_from_ref(&foo);

// We try to increase RC of a pointer that is not Arc-allocated.
let arced: Arc<Foo> = ArcFFI::cloned_from_ptr(ptr);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make this commit a part of follow-up PR, so there is more context why this change makes sense.

@muzarski
Copy link
Collaborator Author

v1.1: Dropped future: pass future pointer to callback directly commit. It will be part of follow-up PR.

@muzarski muzarski requested a review from wprzytula March 12, 2025 15:18
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.

In previous PR, there was a suggestion to introduce a test case for this.
However, if this upgrade returns `None`, this indicates a serious bug in the driver. This is because
both `CassTableMeta` and `CassMeterializedViewMeta` are owned by `CassSchemaMeta`.
The upgrade would return None only if someone freed `CassSchemaMeta` (cass_schema_meta_free).
But in such case, the corresponding CassMaterializedViewMeta was freed as well.
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.

Also removing the outdated comment regarding &'static reference.
@muzarski muzarski merged commit 40e1b17 into scylladb:master Mar 13, 2025
11 checks passed
@muzarski muzarski deleted the pointer-fixes branch March 13, 2025 15:58
@muzarski muzarski mentioned this pull request Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants