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

Resolve UB found by miri #46

Merged
merged 1 commit into from
Aug 27, 2020
Merged

Resolve UB found by miri #46

merged 1 commit into from
Aug 27, 2020

Conversation

KamilaBorowska
Copy link
Contributor

Fixes #41. Fixes #44.

@KamilaBorowska KamilaBorowska force-pushed the resolve-ub branch 2 times, most recently from 69df322 to 08daddc Compare February 18, 2020 19:02
@KamilaBorowska
Copy link
Contributor Author

KamilaBorowska commented Feb 18, 2020 •
edited
Loading

Hm, compatibility with C API may be tricky. Header cannot be packed because it stores atomic integers. I don't think unaligned atomic accesses are legal on non-x86 platforms, and even on x86, you cannot legally write them down within Rust. I think changing C API will be necessary.

@KamilaBorowska
Copy link
Contributor Author

KamilaBorowska commented Feb 18, 2020 •
edited
Loading

This change breaks C ABI compatibility (requiring new tendril.h to link with this version of tendril), keep that in mind. I don't think this soundness change can be applied without doing so - unaligned atomic accesses are not legal.

@KamilaBorowska
Copy link
Contributor Author

Unless... only Header<Atomic> would be incompatible, but not Header<NonAtomic>. I think that may be possible to support.

@KamilaBorowska
Copy link
Contributor Author

Okay, this introduces a breaking change where Tendril<_, Atomic> is no longer C safe. Tendril<_, NonAtomic> is still fine. The C API doesn't allow to create Tendril<_, Atomic>, so this should be acceptable, I hope.

@KamilaBorowska
Copy link
Contributor Author

KamilaBorowska commented Feb 18, 2020 •
edited
Loading

Wait, no, Header cannot be packed, period. This is because we use let shared = (ptr & 1) == 1; for determining if memory is shared, but packed memory has alignment of 1. I suppose I can use align(4) to force workable alignment with packed?

@KamilaBorowska
Copy link
Contributor Author

KamilaBorowska commented Feb 18, 2020 •
edited
Loading

Okay, I think that's it. Miri found legitimate bugs in the library: packed struct where the program depended on particular alignment (as it stored shared bit within the address) and unaligned atomic writes. An alignment aware global allocator (not default malloc) or non-x86 architecture would have broken the library (and of course, as with UB, LLVM upgrades also could).

@SimonSapin
Copy link
Member

I’d be inclined to drop the C API. I don’t know of anyone using it, and it’s not really maintained.

SimonSapin
SimonSapin reviewed Feb 18, 2020
View reviewed changes
src/tendril.rs Outdated Show resolved Hide resolved
@KamilaBorowska
Copy link
Contributor Author

As for C API, I think it should be removed with 0.5.0, but it's not necessary to remove it for this change.

@KamilaBorowska KamilaBorowska force-pushed the resolve-ub branch 7 times, most recently from 3685fec to 9c05ddb Compare February 19, 2020 19:56
@KamilaBorowska
Resolve UB found by miri
9e6da0d 
Fixes servo#41. Fixes servo#44.

This makes `Tendril<_, Atomic>` no longer FFI-safe.
@KamilaBorowska
Copy link
Contributor Author

Updated to run thread tests on miri, now that Miri supports threads.

jdm
jdm reviewed Jun 4, 2020
View reviewed changes
src/tendril.rs
(*self.buf.get()).heap.aux
}

unsafe fn set_aux(&self, aux: u32) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be &mut self like set_len?

Copy link
Contributor Author

@KamilaBorowska KamilaBorowska Aug 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No as it's used by make_buf_shared which is used by public unsafe_subtendril method which accepts &self. Not that this matters because set_aux isn't a public method anyway.

@jdm
Copy link
Member

jdm commented Aug 27, 2020

@bors-servo r+
Thanks!

@bors-servo
Copy link
Contributor

📌 Commit 9e6da0d has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit 9e6da0d with merge 9532724...

@bors-servo
Copy link
Contributor

☀️ Test successful - checks-travis
Approved by: jdm
Pushing 9532724 to master...

@bors-servo bors-servo merged commit 9532724 into servo:master Aug 27, 2020
@SimonSapin SimonSapin mentioned this pull request Jan 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm jdm left review comments

@SimonSapin SimonSapin Awaiting requested review from SimonSapin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use an union to represent inline allocation Resolve UB found by miri
4 participants
@KamilaBorowska @SimonSapin @jdm @bors-servo