Merge pull request #149 from WebAssembly/fix-reentrancy

Catch destructor reentrance

Author: lukewagner

design/mvp/CanonicalABI.md

@@ -219,7 +219,6 @@ class Context:
   opts: CanonicalOptions
   inst: ComponentInstance
   call: Call
-  called_as_export: bool
 ```
 
 The `opts` field represents the [`canonopt`] values supplied to
@@ -250,20 +249,17 @@ class ComponentInstance:
     self.handles = HandleTable()
 ```
 
-Lastly, the `called_as_export` field of `Context` indicates whether the lifted
-function is being called through a component export or whether this is an
-internal call (for example, when a child component calls an import that is
-defined by its parent component).
-
 The `HandleTable` class is defined in terms of a collection of supporting
 runtime bookkeeping classes that we'll go through first.
 
 The `Resource` class represents a runtime instance of a resource type, storing
-the core representation value (which is currently fixed to `i32`):
+the core representation value (which is currently fixed to `i32`) and the
+component instance that is implementing this resource.
 ```python
 @dataclass
 class Resource:
   rep: int
+  impl: ComponentInstance
 ```
 
 The `OwnHandle` and `BorrowHandle` classes represent runtime handle values of
@@ -1422,11 +1418,7 @@ component*.
 Given the above closure arguments, `canon_lift` is defined:
 ```python
 def canon_lift(cx, callee, ft, call, args):
-  if cx.called_as_export:
-    trap_if(not cx.inst.may_enter)
-    cx.inst.may_enter = False
-  else:
-    assert(not cx.inst.may_enter)
+  trap_if(not cx.inst.may_enter)
 
   outer_call = cx.call
   cx.call = call
@@ -1447,9 +1439,6 @@ def canon_lift(cx, callee, ft, call, args):
     if cx.opts.post_return is not None:
       cx.opts.post_return(flat_results)
 
-    if cx.called_as_export:
-      cx.inst.may_enter = True
-
     cx.call.finish_lift()
     cx.call = outer_call
 
@@ -1462,14 +1451,6 @@ boundaries. Thus, if a component wishes to signal an error, it must use some
 sort of explicit type such as `result` (whose `error` case particular language
 bindings may choose to map to and from exceptions).
 
-By clearing `may_enter` for the duration of `canon_lift` when the function is
-called as an export, the dynamic traps ensure that components cannot be
-reentered, ensuring the non-reentrance [component invariant]. Furthermore,
-because `may_enter` is not cleared on the exceptional exit path taken by
-`trap()`, if there is a trap during Core WebAssembly execution of lifting or
-lowering, the component is left permanently un-enterable, ensuring the
-lockdown-after-trap [component invariant].
-
 The `call` parameter is assumed to have been created by the caller (the host or
 `canon lower`) for this one call. Since, in `not cx.called_as_export` scenarios
 a single component instance may be reentered (by its children), `cx.call` must
@@ -1500,9 +1481,13 @@ Thus, from the perspective of Core WebAssembly, `$f` is a [function instance]
 containing a `hostfunc` that closes over `$opts`, `$inst`, `$callee` and `$ft`
 and, when called from Core WebAssembly code, calls `canon_lower`, which is defined as:
 ```python
-def canon_lower(cx, callee, ft, flat_args):
+def canon_lower(cx, callee, calling_import, ft, flat_args):
   trap_if(not cx.inst.may_leave)
 
+  assert(cx.inst.may_enter)
+  if calling_import:
+    cx.inst.may_enter = False
+
   outer_call = cx.call
   cx.call = Call()
 
@@ -1520,6 +1505,9 @@ def canon_lower(cx, callee, ft, flat_args):
   cx.call.finish_lower()
   cx.call = outer_call
 
+  if calling_import:
+    cx.inst.may_enter = True
+
   return flat_results
 ```
 The definitions of `canon_lift` and `canon_lower` are mostly symmetric (swapping
@@ -1538,6 +1526,20 @@ compilation of the permissive [subtyping](Subtyping.md) allowed between
 components (including the elimination of string operations on the labels of
 records and variants) as well as post-MVP [adapter functions].
 
+By clearing `may_enter` for the duration of calls to imports, the `may_enter`
+guard in `canon_lift` ensures that components cannot be externally reentered,
+which is part of the [component invariants]. The `calling_import` condition
+allows a parent component to call into a child component (which is, by
+definition, not a call to an import) and for the child to then reenter the
+parent through a function the parent explicitly supplied to the child's
+`instantiate`. This form of internal reentrance allows the parent to fully
+virtualize the child's imports.
+
+Because `may_enter` is not cleared on the exceptional exit path taken by
+`trap()`, if there is a trap during Core WebAssembly execution of lifting or
+lowering, the component is left permanently un-enterable, ensuring the
+lockdown-after-trap [component invariant].
+
 The `may_leave` flag set during lowering in `canon_lift` and `canon_lower`
 ensures that the relative ordering of the side effects of `lift` and `lower`
 cannot be observed via import calls and thus an implementation may reliably
@@ -1567,7 +1569,7 @@ Calling `$f` invokes the following function, which creates a resource object
 and inserts it into the current instance's handle table:
 ```python
 def canon_resource_new(cx, rt, rep):
-  h = OwnHandle(Resource(rep), rt)
+  h = OwnHandle(Resource(rep, cx.inst), rt)
   return cx.inst.handles.insert(cx, h)
 ```
 
@@ -1588,8 +1590,11 @@ optional destructor.
 def canon_resource_drop(cx, t, i):
   h = cx.inst.handles.remove(cx, i, t)
   if isinstance(t, Own) and t.rt.dtor:
+    trap_if(not h.resource.impl.may_enter)
     t.rt.dtor(h.resource.rep)
 ```
+The `may_enter` guard ensures the non-reentrance [component invariant], since
+a destructor call is analogous to a call to an export.
 
 ### `canon resource.rep`
 
@@ -1618,6 +1623,7 @@ def canon_resource_rep(cx, rt, i):
 [`canonopt`]: Explainer.md#canonical-definitions
 [`canon`]: Explainer.md#canonical-definitions
 [Type Definitions]: Explainer.md#type-definitions
+[Component Invariant]: Explainer.md#component-invariants
 [Component Invariants]: Explainer.md#component-invariants
 [JavaScript Embedding]: Explainer.md#JavaScript-embedding
 [Adapter Functions]: FutureFeatures.md#custom-abis-via-adapter-functions

design/mvp/canonical-abi/definitions.py

@@ -291,7 +291,6 @@ class Context:
   opts: CanonicalOptions
   inst: ComponentInstance
   call: Call
-  called_as_export: bool
 
 #
 
@@ -318,6 +317,7 @@ def __init__(self):
 @dataclass
 class Resource:
   rep: int
+  impl: ComponentInstance
 
 #
 
@@ -1151,11 +1151,7 @@ def lower_values(cx, max_flat, vs, ts, out_param = None):
 ### `lift`
 
 def canon_lift(cx, callee, ft, call, args):
-  if cx.called_as_export:
-    trap_if(not cx.inst.may_enter)
-    cx.inst.may_enter = False
-  else:
-    assert(not cx.inst.may_enter)
+  trap_if(not cx.inst.may_enter)
 
   outer_call = cx.call
   cx.call = call
@@ -1176,19 +1172,20 @@ def post_return():
     if cx.opts.post_return is not None:
       cx.opts.post_return(flat_results)
 
-    if cx.called_as_export:
-      cx.inst.may_enter = True
-
     cx.call.finish_lift()
     cx.call = outer_call
 
   return (results, post_return)
 
 ### `lower`
 
-def canon_lower(cx, callee, ft, flat_args):
+def canon_lower(cx, callee, calling_import, ft, flat_args):
   trap_if(not cx.inst.may_leave)
 
+  assert(cx.inst.may_enter)
+  if calling_import:
+    cx.inst.may_enter = False
+
   outer_call = cx.call
   cx.call = Call()
 
@@ -1206,19 +1203,23 @@ def canon_lower(cx, callee, ft, flat_args):
   cx.call.finish_lower()
   cx.call = outer_call
 
+  if calling_import:
+    cx.inst.may_enter = True
+
   return flat_results
 
 ### `resource.new`
 
 def canon_resource_new(cx, rt, rep):
-  h = OwnHandle(Resource(rep), rt)
+  h = OwnHandle(Resource(rep, cx.inst), rt)
   return cx.inst.handles.insert(cx, h)
 
 ### `resource.drop`
 
 def canon_resource_drop(cx, t, i):
   h = cx.inst.handles.remove(cx, i, t)
   if isinstance(t, Own) and t.rt.dtor:
+    trap_if(not h.resource.impl.may_enter)
     t.rt.dtor(h.resource.rep)
 
 ### `resource.rep`

design/mvp/canonical-abi/run_tests.py

@@ -356,7 +356,7 @@ def test_roundtrip(t, v):
   caller_cx = mk_cx(caller_heap.memory, 'utf8', caller_heap.realloc)
 
   flat_args = lower_flat(caller_cx, v, t)
-  flat_results = canon_lower(caller_cx, lifted_callee, ft, flat_args)
+  flat_results = canon_lower(caller_cx, lifted_callee, True, ft, flat_args)
   got = lift_flat(caller_cx, ValueIter(flat_results), t)
 
   if got != v:
@@ -381,17 +381,19 @@ def dtor(x):
     nonlocal dtor_value
     dtor_value = x
   rt = ResourceType(dtor)
-  r1 = Resource(42)
-  r2 = Resource(43)
-  r3 = Resource(44)
+
+  cx = mk_cx()
+  r1 = Resource(42, cx.inst)
+  r2 = Resource(43, cx.inst)
+  r3 = Resource(44, cx.inst)
 
   def host_import(act, args):
+    nonlocal cx
     assert(len(args) == 2)
     assert(args[0] is r1)
     assert(args[1] is r3)
-    return ([Resource(45)], lambda:())
+    return ([Resource(45, cx.inst)], lambda:())
 
-  cx = mk_cx()
   def core_wasm(args):
     nonlocal dtor_value
 
@@ -404,7 +406,7 @@ def core_wasm(args):
     assert(canon_resource_rep(cx, rt, 2) == 44)
 
     host_ft = FuncType([Borrow(rt),Borrow(rt)],[Own(rt)])
-    results = canon_lower(cx, host_import, host_ft, [Value('i32',0),Value('i32',2)])
+    results = canon_lower(cx, host_import, True, host_ft, [Value('i32',0),Value('i32',2)])
     assert(len(results) == 1)
     assert(results[0].t == 'i32' and results[0].v == 3)
     assert(canon_resource_rep(cx, rt, 3) == 45)