Stream: git-wasmtime

Topic: wasmtime / PR #1624 Revamp memory management of `Instance...

Wasmtime GitHub notifications bot (Apr 28 2020 at 22:00):

alexcrichton opened PR #1624 from fix-segfault to master:

This commit fixes a known but in Wasmtime where an instance could still
be used after it was freed. Unfortunately the fix here is a bit of a
hammer, but it's the best that we can do for now. The changes made in
this commit are:

• A Store now stores all InstanceHandle objects it ever creates.
  This keeps all instances alive unconditionally (along with all host
  functions and such) until the Store is itself dropped. Note that a
  Store is reference counted so basically everything has to be dropped
  to drop anything, there's no longer any partial deallocation of instances.

• The InstanceHandle type's own reference counting has been removed.
  This is largely redundant with what's already happening in Store, so
  there's no need to manage two reference counts.

• Each InstanceHandle no longer tracks its dependencies in terms of
  instance handles. This set was actually inaccurate due to dynamic
  updates to tables and such, so we needed to revamp it anyway.

• Initialization of an InstanceHandle is now deferred until after
  InstanceHandle::new. This allows storing the InstanceHandle before
  side-effectful initialization, such as copying element segments or
  running the start function, to ensure that regardless of the result of
  instantiation the underlying InstanceHandle is still available to
  persist in storage.

Overall this should fix a known possible way to safely segfault Wasmtime
today (yay!) and it should also fix some flaikness I've seen on CI.
Turns out one of the spec tests
(bulk-memory-operations/partial-init-table-segment.wast) exercises this
functionality and we were hitting sporating use-after-free, but only on
Windows.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Apr 28 2020 at 22:01):

alexcrichton edited PR #1624 from fix-segfault to master:

This commit fixes a known but in Wasmtime where an instance could still
be used after it was freed. Unfortunately the fix here is a bit of a
hammer, but it's the best that we can do for now. The changes made in
this commit are:

• A Store now stores all InstanceHandle objects it ever creates.
  This keeps all instances alive unconditionally (along with all host
  functions and such) until the Store is itself dropped. Note that a
  Store is reference counted so basically everything has to be dropped
  to drop anything, there's no longer any partial deallocation of instances.

• The InstanceHandle type's own reference counting has been removed.
  This is largely redundant with what's already happening in Store, so
  there's no need to manage two reference counts.

• Each InstanceHandle no longer tracks its dependencies in terms of
  instance handles. This set was actually inaccurate due to dynamic
  updates to tables and such, so we needed to revamp it anyway.

• Initialization of an InstanceHandle is now deferred until after
  InstanceHandle::new. This allows storing the InstanceHandle before
  side-effectful initialization, such as copying element segments or
  running the start function, to ensure that regardless of the result of
  instantiation the underlying InstanceHandle is still available to
  persist in storage.

Overall this should fix a known possible way to safely segfault Wasmtime
today (yay!) and it should also fix some flaikness I've seen on CI.
Turns out one of the spec tests
(bulk-memory-operations/partial-init-table-segment.wast) exercises this
functionality and we were hitting sporating use-after-free, but only on
Windows.

Closes https://github.com/bytecodealliance/wasmtime/issues/777

Wasmtime GitHub notifications bot (Apr 28 2020 at 22:01):

alexcrichton edited PR #1624 from fix-segfault to master:

This commit fixes a known but in Wasmtime where an instance could still
be used after it was freed. Unfortunately the fix here is a bit of a
hammer, but it's the best that we can do for now. The changes made in
this commit are:

• A Store now stores all InstanceHandle objects it ever creates.
  This keeps all instances alive unconditionally (along with all host
  functions and such) until the Store is itself dropped. Note that a
  Store is reference counted so basically everything has to be dropped
  to drop anything, there's no longer any partial deallocation of instances.

• The InstanceHandle type's own reference counting has been removed.
  This is largely redundant with what's already happening in Store, so
  there's no need to manage two reference counts.

• Each InstanceHandle no longer tracks its dependencies in terms of
  instance handles. This set was actually inaccurate due to dynamic
  updates to tables and such, so we needed to revamp it anyway.

• Initialization of an InstanceHandle is now deferred until after
  InstanceHandle::new. This allows storing the InstanceHandle before
  side-effectful initialization, such as copying element segments or
  running the start function, to ensure that regardless of the result of
  instantiation the underlying InstanceHandle is still available to
  persist in storage.

Overall this should fix a known possible way to safely segfault Wasmtime
today (yay!) and it should also fix some flaikness I've seen on CI.
Turns out one of the spec tests
(bulk-memory-operations/partial-init-table-segment.wast) exercises this
functionality and we were hitting sporating use-after-free, but only on
Windows.

Closes https://github.com/bytecodealliance/wasmtime/issues/777
Closes https://github.com/bytecodealliance/wasmtime/issues/960

Wasmtime GitHub notifications bot (Apr 28 2020 at 22:07):

alexcrichton updated PR #1624 from fix-segfault to master:

This commit fixes a known but in Wasmtime where an instance could still
be used after it was freed. Unfortunately the fix here is a bit of a
hammer, but it's the best that we can do for now. The changes made in
this commit are:

• A Store now stores all InstanceHandle objects it ever creates.
  This keeps all instances alive unconditionally (along with all host
  functions and such) until the Store is itself dropped. Note that a
  Store is reference counted so basically everything has to be dropped
  to drop anything, there's no longer any partial deallocation of instances.

• The InstanceHandle type's own reference counting has been removed.
  This is largely redundant with what's already happening in Store, so
  there's no need to manage two reference counts.

• Each InstanceHandle no longer tracks its dependencies in terms of
  instance handles. This set was actually inaccurate due to dynamic
  updates to tables and such, so we needed to revamp it anyway.

• Initialization of an InstanceHandle is now deferred until after
  InstanceHandle::new. This allows storing the InstanceHandle before
  side-effectful initialization, such as copying element segments or
  running the start function, to ensure that regardless of the result of
  instantiation the underlying InstanceHandle is still available to
  persist in storage.

Overall this should fix a known possible way to safely segfault Wasmtime
today (yay!) and it should also fix some flaikness I've seen on CI.
Turns out one of the spec tests
(bulk-memory-operations/partial-init-table-segment.wast) exercises this
functionality and we were hitting sporating use-after-free, but only on
Windows.

Closes https://github.com/bytecodealliance/wasmtime/issues/777
Closes https://github.com/bytecodealliance/wasmtime/issues/960

Wasmtime GitHub notifications bot (Apr 29 2020 at 00:28):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2020 at 00:28):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2020 at 00:28):

sunfishcode created PR Review Comment:

Typo: InstnaceHandle

Wasmtime GitHub notifications bot (Apr 29 2020 at 00:28):

sunfishcode created PR Review Comment:

Is this meant to be exported from the crate? It doesn't seem to be in lib.rs. If not, can this be pub(crate)?

Wasmtime GitHub notifications bot (Apr 29 2020 at 00:28):

sunfishcode created PR Review Comment:

Could you add a brief comment here mentioning that the Weak is just to break a reference cycle, and that it's always meant to be upgradable in practice?

Wasmtime GitHub notifications bot (Apr 29 2020 at 16:21):

alexcrichton updated PR #1624 from fix-segfault to master:

This commit fixes a known but in Wasmtime where an instance could still
be used after it was freed. Unfortunately the fix here is a bit of a
hammer, but it's the best that we can do for now. The changes made in
this commit are:

• A Store now stores all InstanceHandle objects it ever creates.
  This keeps all instances alive unconditionally (along with all host
  functions and such) until the Store is itself dropped. Note that a
  Store is reference counted so basically everything has to be dropped
  to drop anything, there's no longer any partial deallocation of instances.

• The InstanceHandle type's own reference counting has been removed.
  This is largely redundant with what's already happening in Store, so
  there's no need to manage two reference counts.

• Each InstanceHandle no longer tracks its dependencies in terms of
  instance handles. This set was actually inaccurate due to dynamic
  updates to tables and such, so we needed to revamp it anyway.

• Initialization of an InstanceHandle is now deferred until after
  InstanceHandle::new. This allows storing the InstanceHandle before
  side-effectful initialization, such as copying element segments or
  running the start function, to ensure that regardless of the result of
  instantiation the underlying InstanceHandle is still available to
  persist in storage.

Overall this should fix a known possible way to safely segfault Wasmtime
today (yay!) and it should also fix some flaikness I've seen on CI.
Turns out one of the spec tests
(bulk-memory-operations/partial-init-table-segment.wast) exercises this
functionality and we were hitting sporating use-after-free, but only on
Windows.

Closes https://github.com/bytecodealliance/wasmtime/issues/777
Closes https://github.com/bytecodealliance/wasmtime/issues/960

Wasmtime GitHub notifications bot (Apr 29 2020 at 17:47):

alexcrichton merged PR #1624.

Last updated: Nov 22 2024 at 16:03 UTC