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

Safety of storing mutable objects in collections? #337

Closed
madsmtm opened this issue Jan 12, 2023 · 3 comments · Fixed by #505 or #626
Closed

Safety of storing mutable objects in collections? #337

madsmtm opened this issue Jan 12, 2023 · 3 comments · Fixed by #505 or #626
Labels
A-framework Affects the framework crates and the translator for them I-unsound A soundness hole
Milestone

Comments

@madsmtm
Copy link
Owner

madsmtm commented Jan 12, 2023

I just happened to read this

Storing mutable objects in collection objects can cause problems. Certain collections can become invalid or even corrupt if objects they contain mutate because, by mutating, these objects can affect the way they are placed in the collection. First, the properties of objects that are keys in hashing collections such as NSDictionary objects or NSSet objects will, if changed, corrupt the collection if the changed properties affect the results of the object’s hash or isEqual: methods. (If the hash method of the objects in the collection does not depend on their internal state, corruption is less likely.) Second, if an object in an ordered collection such as a sorted array has its properties changed, this might affect how the object compares to other objects in the array, thus rendering the ordering invalid.

Which is just... Ugh!

So yeah, we may need to manually define which types are safe to use as hash keys - though it might not be that bad, since you usually just want to use NSString or NSNumber anyways.

@madsmtm madsmtm added A-framework Affects the framework crates and the translator for them I-unsound A soundness hole labels Jan 12, 2023
@madsmtm madsmtm added this to the icrate v0.1.0 milestone Apr 21, 2023
@madsmtm
Copy link
Owner Author

madsmtm commented Sep 5, 2023

Something difficult about this is that we want to (unsafely?) allow NSObject/AnyObject as the key, while still having get, iteration and such (because you need access to the elements to be able to later use isKindOfClass: to determine that something was actually a NSString).

Maybe we could just allow NSObject/AnyObject specially?

Or maybe have a kind of "initialization invariant": If you hold a NSDictionary<NSObject, ...>, then you know (?) it was constructed from immutable types, since otherwise it'd be unsound? That at least helps with getting the elements out, though I'm unsure of how to encode that in the type-system?

@madsmtm
Copy link
Owner Author

madsmtm commented Sep 5, 2023

Maybe it's actually just insertion that should be blocked on IsImmutable (or similar), although that may make it difficult to construct property lists? Though then again, they're actually NSDictionary<NSString, NSObject>, i.e. the key is always a string (at least I'm fairly certain).

Linking information about plists:

madsmtm added a commit that referenced this issue Jun 4, 2024
This was added in an abundance of caution in #505 to fix #337, but
prevents real-world usage of these types, and isn't actually needed for
soundness (the documentation mentions the collection being "corrupt" if
the hash is changed, but that's not the same as it being UB).
@madsmtm
Copy link
Owner Author

madsmtm commented Jun 4, 2024

I've reverted the original solution for this (adding a trait HasStableHash that hashing collections require on their parameters) in #626, since it's not actually required for soundness, and prevents a lot of real-world use cases for hashing collections.

madsmtm added a commit that referenced this issue Jun 4, 2024
This was added in an abundance of caution in #505 to fix #337, but
prevents real-world usage of these types, and isn't actually needed for
soundness (the documentation mentions the collection being "corrupt" if
the hash is changed, but that's not the same as it being UB).
madsmtm added a commit that referenced this issue Jun 4, 2024
This was added in an abundance of caution in #505 to fix #337, but
prevents real-world usage of these types, and isn't actually needed for
soundness (the documentation mentions the collection being "corrupt" if
the hash is changed, but that's not the same as it being UB).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-framework Affects the framework crates and the translator for them I-unsound A soundness hole
Projects
None yet
1 participant