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: UnwindRegistry owns the eh_frames #1583

Closed
wants to merge 2 commits into from

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Aug 31, 2020

UnwindRegistry registers eh_frame from a slice. This slice is a
pointer owned by SerializableModule, which is held by Artifact. If
the artifact is dropped before the UnwindRegistry, then we have a
dangling pointer, and it can crash. Actually, it crashes, see
#1581 as our first attempt to
fix this problem.

So instead of UnwindRegistry to register a pointer to a data it
doesn't own, this patch updates UnwindRegistry to register a pointer
to data that it owns. UnwindRegistry now receives an eh_frame of
kind Vec<u8> instead of &[u8]. The bytes are copied.

In addition, this patch creates an UnwindRegistryExt trait, so that
it brings consistency amongst all UnwindRegistry implementation
(systemv, windows_x64 and dummy). This is for an internal usage
only.

Note that this PR fixes the problem I've with python-ext-wasm, but I
would be very surprised if it doesn't break existing tests.

cf #1568
cc @syrusakbary @sebpuetz

`UnwindRegistry` registers `eh_frame` from a slice. This slice is a
pointer owned by `SerializableModule`, which is held by `Artifact`. If
the artifact is dropped _before_ the `UnwindRegistry`, then we have a
dangling pointer, and it can crash. Actually, it crashes, see
wasmerio#1581 as our first attempt to
fix this problem.

So instead of `UnwindRegistry` to register a pointer to a data it
doesn't own, this patch updates `UnwindRegistry` to register a pointer
to data that it owns. `UnwindRegistry` now receives an `eh_frame` of
kind `Vec<u8>` instead of `&[u8]`. The bytes are copied.

In addition, this patch creates an `UnwindRegistryExt` trait, so that
it brings consistency amongst all `UnwindRegistry` implementation
(`systemv`, `windows_x64` and `dummy`). This is for an internal usage
only.
@Hywan Hywan added the bug Something isn't working label Aug 31, 2020
@@ -1,3 +1,20 @@
use wasmer_compiler::CompiledFunctionUnwindInfo;

pub trait UnwindRegistryExt {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced we need a trait, only one implementation would be needed at runtime (or compiled in the code).

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use impl Trait for this if the concern is correctness and efficiency. If the concern is over abstraction though, then that doesn't fix it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The concern is correctness. It declares an interface that all unwind registry implementations must fulfill.

@Hywan
Copy link
Contributor Author

Hywan commented Sep 1, 2020

bors try

bors bot added a commit that referenced this pull request Sep 1, 2020
@sebpuetz
Copy link

sebpuetz commented Sep 1, 2020

I think there's a more fundamental issue about the allocate_custom_sections fn that enabled these bugs in the first place:

    pub(crate) fn allocate_custom_sections(
        &mut self,
        custom_sections: &PrimaryMap<SectionIndex, CustomSection>,
    ) -> Result<PrimaryMap<SectionIndex, *const u8>, CompileError> 

With this signature, it's hands-off as soon as the allocation is done. There's nothing tying the *const u8s to any lifetime.

It might be possible to do it differently tho:

  1. Allocate the custom sections, handing out indices into the CodeMemory's Vec<Vec<u8>>.
    • requries changes CodeMemory
  2. Get the custom sections from the CodeMemory and put them into a PrimaryMap<SectionIndex, &[u8]>.
    • requries changes CodeMemory
  3. Where necessary, convert the &[u8] to Vec<u8> or keep the lifetimes on the structs referencing the allocation.
    • hopefully prevents use-after-free bugs.

@nlewycky nlewycky self-assigned this Sep 1, 2020
@bors
Copy link
Contributor

bors bot commented Sep 1, 2020

try

Timed out.

@MarkMcCaskey
Copy link
Contributor

I think @sebpuetz is correct. I didn't realize that ModuleInfo no longer owned the custom section data anymore but that seems to be a fundamental issue. It's likely non-trivial to make this work without that, so I think the best course of action is to make sure ModuleInfo can stand on its own.

@Hywan
Copy link
Contributor Author

Hywan commented Sep 21, 2020

Outdated by #1628.

@Hywan Hywan closed this Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants