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.

Sign up for GitHub

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

Jump to bottom

libc::clone() should take an unsafe callback #2198

Open
jstarks opened this issue May 26, 2021 · 10 comments · May be fixed by #4341
Open

libc::clone() should take an unsafe callback #2198

jstarks opened this issue May 26, 2021 · 10 comments · May be fixed by #4341
Labels
breakage-candidate C-bug Category: bug E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Milestone
1.0

Comments

@jstarks
Copy link

jstarks commented May 26, 2021

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.
@jstarks jstarks added the C-bug Category: bug label May 26, 2021
@JohnTitor
Copy link
Member

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
Copy link
Author

jstarks commented Jun 1, 2021

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
Copy link
Member

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

@jstarks
Copy link
Author

jstarks commented Jun 1, 2021

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
Copy link
Member

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
Copy link

pro465 commented Nov 5, 2021

Yup, it'll be a breaking change.

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

@joshtriplett
Copy link
Member

You don't have to use the context parameter.

@pro465
Copy link

pro465 commented Nov 19, 2021 •
edited
Loading

@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
Copy link
Contributor

tgross35 commented Aug 29, 2024 •
edited
Loading

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

@tgross35 tgross35 added this to the 1.0 milestone Aug 29, 2024
@tgross35 tgross35 added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Aug 29, 2024
@purplesyringa
Copy link
Contributor

purplesyringa commented Sep 5, 2024 •
edited
Loading

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.

@highjeans highjeans linked a pull request Mar 19, 2025 that will close this issue
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breakage-candidate C-bug Category: bug E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Milestone
1.0
Development

Successfully merging a pull request may close this issue.

Use unsafe callbacks highjeans/libc
6 participants
@joshtriplett @jstarks @tgross35 @purplesyringa @JohnTitor @pro465