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

Soundness fixes #128

Merged
merged 2 commits into from
Feb 28, 2022
Merged

Soundness fixes #128

merged 2 commits into from
Feb 28, 2022

Conversation

madsmtm
Copy link
Owner

@madsmtm madsmtm commented Feb 22, 2022

@madsmtm madsmtm added the bug Something isn't working label Feb 22, 2022
@madsmtm
Copy link
Owner Author

madsmtm commented Feb 24, 2022

Didn't really realize this before, but making type OpaqueData = UnsafeCell<...> actually means that Id<T, Shared> is much safer!

Having aliasing Id<T, Owned> and Id<T, Shared> is ofc. still UB, but if you only use Id<T, Shared> and don't implement Send and/or Sync on your type then Objective-C methods are free to mutate the object as much as they want!

(Concretely, this removes a few noalias and readonly attributes from the LLVM IR of e.g. &Object and &Class, just like using an extern type for those would)

@madsmtm
Copy link
Owner Author

madsmtm commented Feb 24, 2022

To consider: Does it make sense for Id to store an UnsafeCell? How about variance?

EDIT: Nope, it's correct as-is.

@madsmtm
Copy link
Owner Author

madsmtm commented Feb 24, 2022

Also, the pointer in Id<T, Shared> may actually have been mutated by Id::clone, so making Object = UnsafeCell is definitely better!

@madsmtm madsmtm marked this pull request as ready for review February 28, 2022 14:58
@madsmtm madsmtm force-pushed the soundness-fixes branch 2 times, most recently from 41a846b to b987506 Compare February 28, 2022 20:34
@madsmtm madsmtm merged commit a16156c into master Feb 28, 2022
@madsmtm madsmtm deleted the soundness-fixes branch February 28, 2022 23:57
@madsmtm madsmtm added this to the objc2 v0.3 milestone Apr 2, 2022
@madsmtm madsmtm added the I-unsound A soundness hole label Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working I-unsound A soundness hole
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant