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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - asset: make HandleUntyped::id private #7076

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - asset: make HandleUntyped::id private #7076

wants to merge 1 commit into from

Conversation

neocturne
Copy link
Contributor

Objective

It is currently possible to break reference counting for assets by creating a strong HandleUntyped and then modifying the id field before dropping the handle. This should not be allowed.

Solution

Change the id field visibility to private and add a getter instead. The same change was previously done for Handle<T> in #6176, but HandleUntyped was forgotten.

Migration Guide

  • Instead of directly accessing the ID of a HandleUntyped as handle.id, use the new getter handle.id().

@neocturne neocturne changed the title asset: make HandleUntyped<T>::id private asset: make HandleUntyped::id private Jan 2, 2023
@IceSentry IceSentry added A-Assets Load files from disk to use for things like images, models, and sounds C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jan 2, 2023
@IceSentry
Copy link
Contributor

The change makes sense, but you need to fix the usage in the entire codebase to pass CI. I think a few examples use this code.

@neocturne
Copy link
Contributor Author

Ah, I forgot about the examples. Should be fixed now.

@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 3, 2023
@james7132 james7132 added this to the 0.10 milestone Jan 4, 2023
@cart
Copy link
Member

cart commented Jan 4, 2023

bors r+

bors bot pushed a commit that referenced this pull request Jan 4, 2023
@neocturne
asset: make HandleUntyped::id private (#7076)
85743ce 
# Objective

It is currently possible to break reference counting for assets by creating a strong `HandleUntyped` and then modifying the `id` field before dropping the handle. This should not be allowed.

## Solution

Change the `id` field visibility to private and add a getter instead. The same change was previously done for `Handle<T>` in #6176, but `HandleUntyped` was forgotten.

---

## Migration Guide

- Instead of directly accessing the ID of a `HandleUntyped` as `handle.id`, use the new getter `handle.id()`.
@bors
Copy link
Contributor

bors bot commented Jan 4, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title asset: make HandleUntyped::id private [Merged by Bors] - asset: make HandleUntyped::id private Jan 4, 2023
@bors bors bot closed this Jan 4, 2023
@neocturne neocturne deleted the handleuntyped-id branch January 6, 2023 00:54
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
@neocturne @alradish
asset: make HandleUntyped::id private (bevyengine#7076)
24f173a 
# Objective

It is currently possible to break reference counting for assets by creating a strong `HandleUntyped` and then modifying the `id` field before dropping the handle. This should not be allowed.

## Solution

Change the `id` field visibility to private and add a getter instead. The same change was previously done for `Handle<T>` in bevyengine#6176, but `HandleUntyped` was forgotten.

---

## Migration Guide

- Instead of directly accessing the ID of a `HandleUntyped` as `handle.id`, use the new getter `handle.id()`.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@neocturne @ItsDoot
asset: make HandleUntyped::id private (bevyengine#7076)
380d815 
# Objective

It is currently possible to break reference counting for assets by creating a strong `HandleUntyped` and then modifying the `id` field before dropping the handle. This should not be allowed.

## Solution

Change the `id` field visibility to private and add a getter instead. The same change was previously done for `Handle<T>` in bevyengine#6176, but `HandleUntyped` was forgotten.

---

## Migration Guide

- Instead of directly accessing the ID of a `HandleUntyped` as `handle.id`, use the new getter `handle.id()`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockersf mockersf mockersf approved these changes

@IceSentry IceSentry IceSentry approved these changes

@james7132 james7132 james7132 approved these changes

@cart cart cart approved these changes

Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
0.10
Development

Successfully merging this pull request may close these issues.
5 participants
@neocturne @IceSentry @cart @james7132 @mockersf