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

Use ManuallyDrop instead of mem::forget #2765

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

Manishearth
Copy link
Contributor

@Manishearth Manishearth commented Oct 18, 2023

The current code is UB and LLVM could choose to reuse the stack slot causing a UAF.

Motivation

UB is bad.

Solution

Don't do that.

@Manishearth Manishearth requested review from hawkw, davidbarsky and a team as code owners October 18, 2023 23:26
@davidbarsky
Copy link
Member

uh oh. i'm headed to the airport for a vacation, but I started the CI for this.

i'm guessing Miri would've caught this?

@Manishearth
Copy link
Contributor Author

no idea!

@hawkw hawkw enabled auto-merge (squash) October 18, 2023 23:44
@hawkw hawkw merged commit 82a22ee into tokio-rs:master Oct 18, 2023
54 checks passed
hawkw pushed a commit that referenced this pull request Oct 19, 2023
The current code is UB and LLVM could choose to reuse the stack slot causing a UAF.

## Motivation

UB is bad.

## Solution

Don't do that.
hawkw added a commit that referenced this pull request Oct 19, 2023
# 0.1.40

This release fixes a potential stack use-after-free in the
`Instrument::into_inner` method. Only uses of this method are affected
by this bug.

### Fixed

- Use `mem::ManuallyDrop` instead of `mem::forget` in
  `Instrument::into_inner` (#2765)

[#2765]: #2765

Thanks to @cramertj and @Manishearth for finding and fixing this issue!
@hawkw
Copy link
Member

hawkw commented Oct 19, 2023

Wait, this is weird...that into_inner function only added the use of ManuallyDrop in https://github.com/tokio-rs/tracing/pull/2562...which should have been reverted (as adding the Drop impl was an unintentionally breaking change). Why didn't the into_inner function just get reverted back to the original implementation, which was safe and did not use ManuallyDrop at all...?

@Manishearth Manishearth deleted the manuallydrop branch October 19, 2023 05:13
@nixpulvis
Copy link

Gotta love functions that include a Safety section which aren't marked as unsafe.

kaffarell pushed a commit to kaffarell/tracing that referenced this pull request Nov 21, 2023
The current code is UB and LLVM could choose to reuse the stack slot causing a UAF.

## Motivation

UB is bad.

## Solution

Don't do that.
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.1.40

This release fixes a potential stack use-after-free in the
`Instrument::into_inner` method. Only uses of this method are affected
by this bug.

### Fixed

- Use `mem::ManuallyDrop` instead of `mem::forget` in
  `Instrument::into_inner` (tokio-rs#2765)

[tokio-rs#2765]: tokio-rs#2765

Thanks to @cramertj and @Manishearth for finding and fixing this issue!
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