Skip to content

Fix bug in CFArray::to_untyped that made the retain count wrong #139

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

Merged
merged 4 commits into from
Nov 28, 2017

Conversation

faern
Copy link
Contributor

@faern faern commented Nov 28, 2017

The following code segfaults on the 0.4.5 release:

CFArray::<CFBoolean>::from_CFTypes(&[]).to_untyped().to_untyped().to_untyped()

Because to_untyped don't increment the retain count, but when the typed array it's created from goes out of scope that one will decrement it.

And since we need to wrap under the get rule (call CFRetain) anyway I did not see any reason to have it as a consuming to_ method. It's more flexible as a borrowing as_ method.

Since you probably don't want to cause API breakage I kept the old version but fixed the bug and added a deprecation notice on it.


This change is Reviewable

@jdm
Copy link
Member

jdm commented Nov 28, 2017

Let's increase the patch version number and publish a new version as part of this PR. Can we add a test, too?

@faern
Copy link
Contributor Author

faern commented Nov 28, 2017

Sure. I'll fix that! Just a moment.

@faern
Copy link
Contributor Author

faern commented Nov 28, 2017

@jdm Done! I also fixed another bug with CFType::instance_of. At least I think it was a bug. @nox thought so too from a short description on IRC.

@faern
Copy link
Contributor Author

faern commented Nov 28, 2017

I realize I only tested as_untyped. As long as we keep the to_untyped version we might want to test that as well maybe? Adding to same test...

@faern faern force-pushed the fix-to-untyped-array branch from 6d88b3d to c33598d Compare November 28, 2017 15:47
@jdm
Copy link
Member

jdm commented Nov 28, 2017

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit c33598d has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit c33598d with merge 665fe0f...

bors-servo pushed a commit that referenced this pull request Nov 28, 2017
Fix bug in CFArray::to_untyped that made the retain count wrong

The following code segfaults on the `0.4.5` release:
```rust
CFArray::<CFBoolean>::from_CFTypes(&[]).to_untyped().to_untyped().to_untyped()
```
Because `to_untyped` don't increment the retain count, but when the typed array it's created from goes out of scope that one will decrement it.

And since we need to wrap under the get rule (call `CFRetain`) anyway I did not see any reason to have it as a consuming `to_` method. It's more flexible as a borrowing `as_` method.

Since you probably don't want to cause API breakage I kept the old version but fixed the bug and added a deprecation notice on it.

<!-- 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/139)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-travis
Approved by: jdm
Pushing 665fe0f to master...

@bors-servo bors-servo merged commit c33598d into servo:master Nov 28, 2017
@faern faern deleted the fix-to-untyped-array branch November 28, 2017 17:15
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.

3 participants