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 wrong pointer type of static _dispatch_main_q binding #343

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MarijnS95
Copy link
Contributor

While looking at a copy of this code (in another project that uses objc2, which does not yet define dispatch integration) this strange cast immediately stands out. _dispatch_main_q is defined here as a dispatch_queue_t, the queue argument in dispatch_data_create() is also defined as a dispatch_queue_t, yet there is a "borrow" (get pointer to) operation before passing it into the function.

Instead, this static field is supposed to be defined as the Object itself, so that any user could take a pointer to it if they wish or need it so. The same is seen in the dispatch crate: _dispatch_main_q is defined as a dispatch_object_s, and dispatch_queue_t is a typedef to a *mut dispatch_object_s to match the above convention.

Note that this is not a bugfix, but merely a readability improvement. Use of mut here is debatable.

@MarijnS95
Copy link
Contributor Author

CI correctly suggests usage of the new &raw syntax, but that requires an MSRV bump to 1.82.

Copy link
Contributor

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

👍

Note that it might technically be a soundness fix, since we now go from telling the compiler that _dispatch_main_q has the size of a pointer (and asserting the validity of that by creating a reference in &_dispatch_main_q), to using Object which is zero-sized, and hence acts as-if the object may have an arbitrary size (in a similar way that extern types do).

But I doubt it matters at all in practice (esp. since LLVM migrated to opaque pointers).

Use of mut here is debatable.

Yeah, if we want to be really correct, I'd recommend something like _dispatch_main_q: SyncUnsafeCell<Object>.

MSRV

You can use std::ptr::addr_of[_mut]! instead (and #[allow(...)] the clippy warning for that).

While looking at a copy of this code (in another project that uses
`objc2`, which does not yet define `dispatch` integration) this strange
cast immediately stands out.  `_dispatch_main_q` is defined here as a
`dispatch_queue_t`, the `queue` argument in `dispatch_data_create()`
is also defined as a `dispatch_queue_t`, yet there is a "borrow" (get
*pointer to*) operation before passing it into the function.

Instead, this `static` field is supposed to be defined as the
`Object` itself, so that any user could take a _pointer to it_
if they wish or need it so.  The same is seen in [the `dispatch`
crate]: `_dispatch_main_q` is defined as a `dispatch_object_s`, and
`dispatch_queue_t` is a _typedef_ to a `*mut dispatch_object_s` to match
the above convention.

Note that this is not a bugfix, but merely a readability improvement.
Use of `mut` here is debatable.

[the `dispatch` crate]: https://github.com/SSheldon/rust-dispatch/blob/f540a2d8ccaebf0e87f5805033b9e287e8d01ba5/src/ffi.rs#L33
@MarijnS95 MarijnS95 force-pushed the static-dispatch-q-type branch from 53270f3 to 780f9b5 Compare January 2, 2025 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants