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

BUG: Add test showing how downcasting to CFArray is unsound #158

Merged
merged 1 commit into from
May 7, 2018

Conversation

faern
Copy link
Contributor

@faern faern commented Feb 1, 2018

This PR is not intended to me merged directly. It adds a test that causes undefined behavior without using unsafe {}. So it's more to show the problem and discuss a solution.

The problem here is that if you have a CFArray<X> you can cast it up to a CFType and then down to a CFArray<Y> for any Y: TCFType. It never checks that the elements in the array are of type Y or not. At this point CFArray::iter will happily give out &Y references, while in fact the pointers backing those objects are pointing to XRef instances. So doing anything with the &Y will likely cause invalid memory access.

How do we solve this?

Ping @jrmuizel


This change is Reviewable

@faern
Copy link
Contributor Author

faern commented Feb 1, 2018

I pushed another test, showing how this problem originates at TCFType::instance_of. Where

CFArray<X>::instance_of::<CFArray<Y>>()

Does return true in all cases.
I don't think the instance_of problem can be used to cause undefined behavior though. But it's still a strange situation.

@jrmuizel
Copy link
Collaborator

jrmuizel commented Feb 1, 2018

Casting a CFArray to a particular CFArray needs to be unsafe. I'm not yet sure how fix downcast.

@jrmuizel
Copy link
Collaborator

jrmuizel commented Feb 1, 2018

If we add a trait that's ConcreteCFType that non-generic CFTypes implement.

We can make downcast become

    pub fn downcast<T: ConcreteCFType>(&self) -> Option<T> {

and then we'd have
impl ConcreteCFType for CFArray<void*>
but not for CFArray

@jrmuizel
Copy link
Collaborator

jrmuizel commented Feb 1, 2018

I've put up a fix in #159

jdm pushed a commit that referenced this pull request Feb 1, 2018
Add appearance names

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/cocoa-rs/158)
<!-- Reviewable:end -->
@jdm
Copy link
Member

jdm commented Feb 2, 2018

We could potentially turn this into a doctest that verifies that it doesn't compile?

@faern
Copy link
Contributor Author

faern commented Feb 3, 2018

@jdm How do you create a test that passes if it does not compile?

@jrmuizel
Copy link
Collaborator

jrmuizel commented Feb 3, 2018

See https://doc.rust-lang.org/beta/rustdoc/documentation-tests.html

/// ```compile_fail
/// let x = 5;
/// x += 2; // shouldn't compile!
/// ```

@faern
Copy link
Contributor Author

faern commented Mar 20, 2018

Updated the test to be a compile_fail doctest. Also fixed the trait bound on downcast_into to prohibit unsound downcasts there as well.

@jrmuizel
Copy link
Collaborator

jrmuizel commented May 7, 2018

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 6c63acc has been approved by jrmuizel

@bors-servo
Copy link
Contributor

⌛ Testing commit 6c63acc with merge 2ba1ea2...

bors-servo pushed a commit that referenced this pull request May 7, 2018
BUG: Add test showing how downcasting to CFArray is unsound

This PR is not intended to me merged directly. It adds a test that causes undefined behavior without using `unsafe {}`. So it's more to show the problem and discuss a solution.

The problem here is that if you have a `CFArray<X>` you can cast it up to a `CFType` and then down to a `CFArray<Y>` for any `Y: TCFType`. It never checks that the elements in the array are of type `Y` or not. At this point `CFArray::iter` will happily give out `&Y` references, while in fact the pointers backing those objects are pointing to `XRef` instances. So doing anything with the `&Y` will likely cause invalid memory access.

How do we solve this?

Ping @jrmuizel

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/core-foundation-rs/158)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-travis
Approved by: jrmuizel
Pushing 2ba1ea2 to master...

@bors-servo bors-servo merged commit 6c63acc into servo:master May 7, 2018
@faern
Copy link
Contributor Author

faern commented May 8, 2018

The downcast_into<T: ConcreteCFType> change here is breaking. But it's just as important for the security as the equivalent on downcast that was performed earlier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants