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

Change how mutability in msg_send! is done #150

Merged
merged 7 commits into from
Jun 6, 2022

Conversation

madsmtm
Copy link
Owner

@madsmtm madsmtm commented May 24, 2022

Resolves upstream issue SSheldon/rust-objc#112.

Whichever solution we choose has quite the large effect on #120, since we'd like msg_send! and msg_send_id! to be consistent, and e.g. msg_send_id! sometimes needs the ability to consume its arguments.

This PR is a stab at implementing idea 3 as outlined in SSheldon/rust-objc#112 (comment). Not quite happy with the fact that we use fully qualified method calls, which means reborrows are sometimes necessary (see diff in objc2-foundation/src/array.rs and objc2-foundation/src/data.rs, may or may not be a big problem in practice?).

I'm reluctant to do idea 2 (add msg_send_mut!) since we would also need msg_send_bool_mut! and msg_send_id_mut! which is quite cumbersome!

@madsmtm madsmtm added bug Something isn't working help wanted Extra attention is needed A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates labels May 24, 2022
@madsmtm madsmtm changed the title Change msg_send! such that callers can properly communicate mutability Change how mutability in msg_send! is done May 24, 2022
@madsmtm madsmtm added this to the objc2 v0.3 milestone May 24, 2022
@madsmtm madsmtm mentioned this pull request May 24, 2022
12 tasks
@madsmtm madsmtm force-pushed the msg-send-receiver-soundness branch from f1edce7 to 763efa1 Compare May 26, 2022 12:59
@madsmtm madsmtm force-pushed the msg-send-receiver-soundness branch from 763efa1 to d026568 Compare June 6, 2022 15:16
@madsmtm madsmtm marked this pull request as ready for review June 6, 2022 15:16
@madsmtm madsmtm force-pushed the msg-send-receiver-soundness branch from d026568 to 854b252 Compare June 6, 2022 15:34
@madsmtm
Copy link
Owner Author

madsmtm commented Jun 6, 2022

I'll go with idea 3, to make msg_send! consume its receiver (and require &[mut] obj when obj is an Id), since that is how msg_send_id! in #120 will end up working anyhow, and then they'll be fully compatible.

@madsmtm madsmtm force-pushed the msg-send-receiver-soundness branch 4 times, most recently from 67d85b6 to 802f8c2 Compare June 6, 2022 15:59
madsmtm added 6 commits June 6, 2022 18:48
This fixes a long-standing soundness issue with how message sending is done whilst mutating the receiver, see: SSheldon/rust-objc#112.

We were effectively mutating behind either `&&mut T` or `&T`, where `T` is zero-sized and contains `UnsafeCell`, so while it is still uncertain exactly how much of an issue this actually is, the approach we use now is definitely sound!

Also makes it clearer that `msg_send!` does not consume `Id`s, it only needs a reference to those.
@madsmtm madsmtm force-pushed the msg-send-receiver-soundness branch 2 times, most recently from 7ba94f9 to 7a94811 Compare June 6, 2022 16:57
If you do not want to consume it, the possibility of doing `msg_send![&*obj]` exists, but most of the time that is indeed what you want (I mean, why else would you wrap it in `ManuallyDrop`?)
@madsmtm madsmtm force-pushed the msg-send-receiver-soundness branch from 7a94811 to 2beef61 Compare June 6, 2022 16:58
@madsmtm madsmtm merged commit dbddcb8 into master Jun 6, 2022
@madsmtm madsmtm deleted the msg-send-receiver-soundness branch June 6, 2022 18:27
@madsmtm madsmtm mentioned this pull request Jun 16, 2022
madsmtm added a commit to madsmtm/cacao that referenced this pull request Jul 20, 2022
madsmtm added a commit to madsmtm/cacao that referenced this pull request Jul 20, 2022
madsmtm added a commit to madsmtm/cacao that referenced this pull request Aug 28, 2022
madsmtm added a commit to madsmtm/cacao that referenced this pull request Aug 1, 2023
madsmtm added a commit to madsmtm/cacao that referenced this pull request Aug 1, 2023
madsmtm added a commit to madsmtm/cacao that referenced this pull request Aug 1, 2023
madsmtm added a commit to madsmtm/cacao that referenced this pull request Sep 5, 2023
ryanmcgrath pushed a commit to ryanmcgrath/cacao that referenced this pull request Sep 11, 2023
* Use objc2

* Replace `objc_id`

* Remove sel_impl import

* Fix `add_method` calls

* Fix accessing raw FFI functions

* Fix Encode impl

* Fix message sends arguments that do not implement `Encode`

* Use immutable reference in a few places where now necessary

See madsmtm/objc2#150 for a bit of background

* Add a few Send + Sync bounds where examples require it

This is something we'll need to look into properly

* Use `&'static Class` instead of `*const Class`

Safer and more ergonomic. Also required for `msg_send_id!` macro

* Use msg_send_id! and rc::Id

* Update objc2 to v0.3.0-beta.2

* Replace `BOOL` with `Bool` when declaring delegates

This makes cacao compile on Aarch64 again

* Remove a few impossible to use correctly `into_inner` functions

These consumed `self`, and hence also dropped `Id` variable that was responsible for keeping the returned pointer alive

* Remove a few impossible to use correctly `From` implementations

* Quickly fix UB with using BACKGROUND_COLOR ivar

* Fix double-freeing of windows

* Fix double freeing of strings

* Fix a few remaining double-frees
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant