libc::clone() should take an unsafe callback

[jstarks]

On Linux, clone takes a callback function pointer. The function pointer type is not marked unsafe, but there is basically no way to implement this callback with a safe function since it takes its context parameter by pointer.

It seems to me that unsafe should be added to the function pointer type, e.g.:

    pub fn clone(
        cb: unsafe extern "C" fn(*mut ::c_void) -> ::c_int,
        child_stack: *mut ::c_void,
        flags: ::c_int,
        arg: *mut ::c_void,
        ...
    ) -> ::c_int;

I'm not certain, but I don't think this would be a breaking change.

[JohnTitor]

I'm not sure why this change is needed as that function should be usable currently. For instance, it's normally used on the user codebase: https://github.com/nix-rust/nix/blob/b4d4b3183b75addf7cad83c57994a62a89235ed4/src/sched.rs#L194-L201

I'm not certain, but I don't think this would be a breaking change.

Yup, it'll be a breaking change.

[jstarks]

Sure, it's possible to work around this via transmute. It just introduces another opportunity for error.

I guess this is a breaking change because someone might care about the type of the clone function, is that right? As long as clone is not used as a function pointer, then this change would not cause any existing program to fail to compile, no?

[JohnTitor]

If it's usable then I don't see any strong reason to change. Could you show some situation it should be changed?

[jstarks]

We can agree it's a bug, right? Just one that can be worked around. I defer to your judgment whether it's worth fixing; feel free to close.

[JohnTitor]

I don't think it's a bug but an improvement, though. So, adding unsafe helps some usage (and it doesn't break any build), I'm happy to accept that change. Therefore I'd like to see an example you consider.

[pro465]

Yup, it'll be a breaking change.

is not libc still 0.x.y, which means it can have breaking changes?

[joshtriplett]

You don't have to use the context parameter.

[pro465]

@JohnTitor

I'm not certain, but I don't think this would be a breaking change.

Yup, it'll be a breaking change.

how? are not safe fn pointers implicitly castable to unsafe fn pointers?

link to working example with the same typed fn pointer as the OP's suggestion:https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&code=use%20std%3A%3Affi%3A%3Ac_void%3B%0Ause%20std%3A%3Aos%3A%3Araw%3A%3Ac_int%3B%0A%0Aextern%20%22C%22%20fn%20foo(%3A%20*mut%20c_void)%20-%3E%20c_int%20%7B%201%20%7D%0A%0Afn%20takes_unsafe_callback(callback%3A%20unsafe%20extern%20%22C%22%20fn(*mut%20c_void)%20-%3E%20c_int)%20%7B%0A%20%20%20%20unsafe%20%7B%20callback(0%20as%20)%3B%20%7D%0A%7D%0A%0Afn%20main()%20%7B%0A%20%20%20%20takes_unsafe_callback(foo%20as%20extern%20%22C%22%20fn(*mut%20c_void)%20-%3E%20c_int)%3B%0A%7D

[tgross35]

Let's just do this for 1.0, along with any other functions that take a callback with pointer arguments. PRs are welcome!

[purplesyringa]

Just for context, the problem is that Rust's best practices require all safe (even internal) functions to be sound when called with arbitrary arguments. However, a callback that doesn't just ignore its argument but casts it to *const UserData and then dereferences the pointer cannot be sound for arbitrary arguments, that is, be safe. So following safety-oriented best practices (marking the callback unsafe fn) requires jumping through extra unsafe hoops (transmuting the callback from unsafe fn to fn at the invocation of clone), which in practice leads to the best practices (if you can even call them that, they are more akin to strict rules) not being followed at all.

I'm glad this is going to be fixed in 1.0.