Skip to content

xpc-sys: Don't xpc_release NULLs#22

Merged
mach-kernel merged 4 commits intomasterfrom
120423/xpc-object-default
Dec 21, 2023
Merged

xpc-sys: Don't xpc_release NULLs#22
mach-kernel merged 4 commits intomasterfrom
120423/xpc-object-default

Conversation

@mach-kernel
Copy link
Collaborator

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
Collaborator 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
Collaborator 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
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