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

Fix undefined behavior in `energymon::init` #26641

Merged
merged 1 commit into from May 26, 2020
Merged

Conversation

@dylni
Copy link
Contributor

dylni commented May 25, 2020

The layout of Box is implicitly #[repr(Rust)], so it is undefined behavior to transmute it to another type. Box::into_raw can safely convert it to a pointer.

I did not run the test suite locally. I thought it would be better to let CI run than to set up the development environment for such a simple change. I did test that the file works individually with this change.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because the undefined behavior is not visible
@highfive
Copy link

highfive commented May 25, 2020

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

@SimonSapin
Copy link
Member

SimonSapin commented May 25, 2020

Looks good, thanks!

The remaining unsafe code in this file still looks very suspicious to me regarding thread-safety, but we can leave that for #26550.

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented May 25, 2020

📌 Commit 3160e62 has been approved by SimonSapin

@highfive highfive assigned SimonSapin and unassigned jdm May 25, 2020
@dylni
Copy link
Contributor Author

dylni commented May 25, 2020

Nice to see the whole static mut being reconsidered! It's interesting how contemporaneous it is with this PR.

@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2020

Testing commit 3160e62 with merge 71d4ea6...

bors-servo added a commit that referenced this pull request May 26, 2020
Fix undefined behavior in `energymon::init`

<!-- Please describe your changes on the following line: -->
The layout of [`Box`](https://doc.rust-lang.org/std/boxed/struct.Box.html) is implicitly [`#[repr(Rust)]`](https://doc.rust-lang.org/nomicon/repr-rust.html), so it is undefined behavior to transmute it to another type. [`Box::into_raw`](https://doc.rust-lang.org/std/boxed/struct.Box.html#method.into_raw) can safely convert it to a pointer.

I did not run the test suite locally. I thought it would be better to let CI run than to set up the development environment for such a simple change. I did test that the file works individually with this change.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors

<!-- Either: -->
- [X] These changes do not require tests because the undefined behavior is not visible

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2020

💔 Test failed - status-taskcluster

@CYBAI
Copy link
Collaborator

CYBAI commented May 26, 2020

bors-servo added a commit that referenced this pull request May 26, 2020
Fix undefined behavior in `energymon::init`

<!-- Please describe your changes on the following line: -->
The layout of [`Box`](https://doc.rust-lang.org/std/boxed/struct.Box.html) is implicitly [`#[repr(Rust)]`](https://doc.rust-lang.org/nomicon/repr-rust.html), so it is undefined behavior to transmute it to another type. [`Box::into_raw`](https://doc.rust-lang.org/std/boxed/struct.Box.html#method.into_raw) can safely convert it to a pointer.

I did not run the test suite locally. I thought it would be better to let CI run than to set up the development environment for such a simple change. I did test that the file works individually with this change.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors

<!-- Either: -->
- [X] These changes do not require tests because the undefined behavior is not visible

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2020

Testing commit 3160e62 with merge b4c846d...

@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member

jdm commented May 26, 2020

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2020

Testing commit 3160e62 with merge 844dd85...

@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2020

☀️ Test successful - status-taskcluster
Approved by: SimonSapin
Pushing 844dd85 to master...

@bors-servo bors-servo merged commit 844dd85 into servo:master May 26, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.