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

When reporting a heap use-after-free, say where the allocation was allocated and deallocated #2940

Merged
merged 3 commits into from
Aug 16, 2023

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Jun 25, 2023

This is a partial solution to: #2917

Currently in the interpreter, we only have accurate information for where heap allocations are allocated and deallocated (see #2940 (comment)). So this just implements support for allocations where the information is already available, and the full support will require more interpreter tweaks.

@RalfJung
Copy link
Member

RalfJung commented Jun 25, 2023 via email

@saethlin
Copy link
Member Author

The primary response to the current diagnostic is "great, now what even is alloc1234" and this change answers that question. I agree that the next question is where the allocation is deallocated, I'll add that.

@RalfJung
Copy link
Member

RalfJung commented Jun 26, 2023 via email

src/diagnostics.rs Outdated Show resolved Hide resolved
src/machine.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

@rustbot author

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Jun 28, 2023
@bors
Copy link
Collaborator

bors commented Aug 3, 2023

☔ The latest upstream changes (presumably #3007) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member

@saethlin what are your plans for this PR?

An MVP of this might be to only track spans for heap allocations for now?

@saethlin
Copy link
Member Author

MVP is a good idea. I'll strip this down into a heap-only MVP then probably open an issue to permit this to work for all allocations.

@saethlin saethlin changed the title When reporting a use-after-free, say where the allocation was allocated When reporting a heap use-after-free, say where the allocation was allocated and deallocated Aug 11, 2023
@saethlin saethlin removed the S-waiting-on-author Status: Waiting for the PR author to address review comments label Aug 11, 2023
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

Looks good, just some nits!

src/machine.rs Outdated
use self::MiriMemoryKind::*;
match self {
Rust | Miri | C | Mmap => true,
Machine | Global | ExternStatic | Tls | WinHeap | Runtime => false,
Copy link
Member

Choose a reason for hiding this comment

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

Why is WinHeap treated different from the other heap allocations?

Copy link
Member Author

Choose a reason for hiding this comment

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

I probably pulled it into the false list then never really noticed the mistake... do we have any tests that would show a use-after-free span for WinHeap? I don't think we actually do.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have such a test, no.

src/machine.rs Outdated Show resolved Hide resolved
src/machine.rs Outdated Show resolved Hide resolved
src/machine.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

Thanks! A nice improvement. :)
@bors r+

@bors
Copy link
Collaborator

bors commented Aug 16, 2023

📌 Commit eadad01 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Aug 16, 2023

⌛ Testing commit eadad01 with merge 1056110...

@bors
Copy link
Collaborator

bors commented Aug 16, 2023

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 1056110 to master...

@bors bors merged commit 1056110 into rust-lang:master Aug 16, 2023
8 checks passed
@saethlin saethlin deleted the use-after-free-spans branch August 16, 2023 10:05
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Aug 17, 2023
When reporting a heap use-after-free, say where the allocation was allocated and deallocated

This is a partial solution to: rust-lang/miri#2917

Currently in the interpreter, we only have accurate information for where heap allocations are allocated and deallocated (see rust-lang/miri#2940 (comment)). So this just implements support for allocations where the information is already available, and the full support will require more interpreter tweaks.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 25, 2023
…lfJung

Record allocation spans inside force_allocation

This expands rust-lang/miri#2940 to cover locals

r? `@RalfJung`
github-actions bot pushed a commit that referenced this pull request Aug 26, 2023
Record allocation spans inside force_allocation

This expands #2940 to cover locals

r? `@RalfJung`
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
When reporting a heap use-after-free, say where the allocation was allocated and deallocated

This is a partial solution to: rust-lang/miri#2917

Currently in the interpreter, we only have accurate information for where heap allocations are allocated and deallocated (see rust-lang/miri#2940 (comment)). So this just implements support for allocations where the information is already available, and the full support will require more interpreter tweaks.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
When reporting a heap use-after-free, say where the allocation was allocated and deallocated

This is a partial solution to: rust-lang/miri#2917

Currently in the interpreter, we only have accurate information for where heap allocations are allocated and deallocated (see rust-lang/miri#2940 (comment)). So this just implements support for allocations where the information is already available, and the full support will require more interpreter tweaks.
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.

None yet

4 participants