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

xpc-sys: Don't xpc_release NULLs #22

Merged
merged 4 commits into from
Dec 21, 2023
Merged

Conversation

mach-kernel
Copy link
Owner

I am not sure that implementing Default for XPCObject makes much sense. Options are:

  • Get rid of the Default impl
  • Return xpc_null_create()
  • Leave as is and check for NULL xpc_object_t (probably a good idea)

Strangely enough nothing bad happens when doing this on Sonoma:

#[test]
fn release_null() {
    unsafe { xpc_release(null_mut()) }
}
running 1 test
test objects::xpc_object::tests::release_null ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 23 filtered out; finished in 0.00s

@hjmallon
Copy link
Contributor

hjmallon commented Dec 4, 2023

Interesting, I am on Sonoma and that is what is showing complaints (I added it to a test and ran cargo test).

If nullptr is never valid for xpc-object-t then you could make the ptr NonNull? I am not sure if null is ever useful?

@mach-kernel
Copy link
Owner Author

Re NonNull: The XPC bindings expect an xpc_object_t (*mut c_void), so it would change things a little, e.g.:

pub fn xpc_copy(xpc_object: NonNull<c_void>) -> Option<Self> {
    // -> *mut c_void
    let clone = unsafe { xpc_copy(xpc_object.as_ptr()) };
    NonNull::new(clone).map(Self::new)
}

What's confusing is that some of the Obj-C signatures use the _Nonnull extension like xpc_dictionary_create, but not xpc_copy:

xpc_object_t xpc_dictionary_create(const char *const _Nonnull *keys, xpc_object_t _Nullable const *values, size_t count);
xpc_object_t xpc_copy(xpc_object_t object);

Some of the documented XPC APIs return NULL xpc_dictionary_create_reply. I need to do some homework on FFI semantics + NonNull. Then go figure out which usages it makes most sense for (here is appetizing). It might be OK to use NonNull<c_void> instead of xpc_object_t in the Rust XPCObject.

At the very least this has convinced me to drop the current Default impl for XPCObject.

@hjmallon
Copy link
Contributor

hjmallon commented Dec 5, 2023

Ok so a good step one would be removing default and also merging this? Then nullptr xpcobjects work, but default doesn’t make them. That would work for what I am doing right not.

to do the more extreme nonnull path would involve more checking in more places and also working out whether it is ever useful to have a nullptr.

@mach-kernel
Copy link
Owner Author

Pardon the delay. I dropped impl Default and will cut a 0.5.0 for xpc-sys. Thanks!

@mach-kernel mach-kernel merged commit 3c2ccf3 into master Dec 21, 2023
1 check passed
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