CABI: check ptr bounds for zero-length reads/writes

Author: lukewagner

design/mvp/CanonicalABI.md

@@ -1551,7 +1551,7 @@ class GuestBuffer(Buffer):
 
   def __init__(self, t, cx, ptr, length):
     trap_if(length > Buffer.MAX_LENGTH)
-    if t and length > 0:
+    if t is not None:
       trap_if(ptr != align_to(ptr, alignment(t, cx.opts.memory.ptr_type())))
       trap_if(ptr + length * elem_size(t, cx.opts.memory.ptr_type()) > len(cx.opts.memory))
     self.cx = cx
@@ -1584,11 +1584,13 @@ class WritableGuestBuffer(GuestBuffer, WritableBuffer):
       assert(all(v == () for v in vs))
     self.progress += len(vs)
 ```
-When `t` is `None` (arising from `stream` and `future` with empty element
-types), the core-wasm-supplied `ptr` is entirely ignored, while the `length`
-and `progress` are still semantically meaningful. Source bindings may represent
-this case with a generic stream/future of [unit] type or a distinct type that
-conveys events without values.
+As with `load_list_from_range` below, the alignment and range checks are
+performed even when `length` is `0`. When `t` is `None` (arising from `stream`
+and `future` with empty element types), the core-wasm-supplied `ptr` is
+entirely ignored, while the `length` and `progress` are still semantically
+meaningful. Source bindings may represent this case with a generic
+stream/future of [unit] type or a distinct type that conveys events without
+values.
 
 The `load_list_from_valid_range` and `store_list_into_valid_range` functions
 that do all the heavy lifting are shared with function parameter/result lifting
@@ -1691,30 +1693,33 @@ the code below:
       assert(self.has_pending_event())
     elif self.other.buffer is None:
       self.buffer = buffer
-    elif buffer.remain() > 0 and self.other.buffer.remain() > 0:
+    else:
       trap_if(self.owner and self.owner is self.other.owner and not none_or_number_type(self.t))
-      n = min(buffer.remain(), self.other.buffer.remain())
-      if is_read:
-        buffer.write(self.other.buffer.read(n))
-      else:
-        self.other.buffer.write(buffer.read(n))
-      self.notify(buffer.progress)
-      self.other.notify(self.other.buffer.progress)
-      if self.other.buffer.remain() == 0:
+      if buffer.remain() > 0 and self.other.buffer.remain() > 0:
+        n = min(buffer.remain(), self.other.buffer.remain())
+        if is_read:
+          buffer.write(self.other.buffer.read(n))
+        else:
+          self.other.buffer.write(buffer.read(n))
+        self.notify(buffer.progress)
+        self.other.notify(self.other.buffer.progress)
+        if self.other.buffer.remain() == 0:
+          self.other.buffer = None
+      elif buffer.remain() > 0 or (is_read and self.other.buffer.remain() == 0):
+        self.other.notify(progress = 0)
         self.other.buffer = None
-    elif buffer.remain() > 0 or (is_read and self.other.buffer.remain() == 0):
-      self.other.notify(progress = 0)
-      self.other.buffer = None
-      self.buffer = buffer
-    else:
-      self.notify(progress = 0)
+        self.buffer = buffer
+      else:
+        self.notify(progress = 0)
 ```
 As a temporary measure (until [lazy lowering] obviates the problem), there is
 also a trap when both the `read` and `write` come from the same component
 instance and there is a non-empty, non-number element type. The reason for this
 trap is that when lifting and lowering can alias the same memory, the eager
 interleaving semantics of copying compound values would otherwise be complex to
-precisely specify and implement.
+precisely specify and implement. The trap is checked whenever both ends have
+provided a buffer (cases 3, 4 and 5), even if no elements end up being copied,
+so that whether a trap occurs doesn't depend on the buffer lengths.
 
 The `End.cancel` method is called by `{stream,future}.cancel-{read,write}` to
 transition an end from `COPYING` (set by `End.copy` above) to `CANCELLING_COPY`.

design/mvp/Explainer.md

@@ -2006,7 +2006,8 @@ enum copy-result {
 The `stream.read` and `stream.write` built-ins take the matching [readable or
 writable end] of a stream as the first parameter and, if `T` is present, a
 buffer for the `T` values to be read from or written to. If `T` is not present,
-the buffer parameter is ignored.
+the buffer parameter is ignored. Otherwise, the buffer's alignment and bounds
+are checked even when its length is `0`.
 
 If the return value is a `stream-result`, then the `progress` field indicates how
 many `T` elements were read or written from the given buffer before the

design/mvp/canonical-abi/definitions.py

@@ -895,7 +895,7 @@ class GuestBuffer(Buffer):
 
   def __init__(self, t, cx, ptr, length):
     trap_if(length > Buffer.MAX_LENGTH)
-    if t and length > 0:
+    if t is not None:
       trap_if(ptr != align_to(ptr, alignment(t, cx.opts.memory.ptr_type())))
       trap_if(ptr + length * elem_size(t, cx.opts.memory.ptr_type()) > len(cx.opts.memory))
     self.cx = cx
@@ -962,23 +962,24 @@ def copy(self, buffer: Buffer, is_read: bool):
       assert(self.has_pending_event())
     elif self.other.buffer is None:
       self.buffer = buffer
-    elif buffer.remain() > 0 and self.other.buffer.remain() > 0:
+    else:
       trap_if(self.owner and self.owner is self.other.owner and not none_or_number_type(self.t))
-      n = min(buffer.remain(), self.other.buffer.remain())
-      if is_read:
-        buffer.write(self.other.buffer.read(n))
-      else:
-        self.other.buffer.write(buffer.read(n))
-      self.notify(buffer.progress)
-      self.other.notify(self.other.buffer.progress)
-      if self.other.buffer.remain() == 0:
+      if buffer.remain() > 0 and self.other.buffer.remain() > 0:
+        n = min(buffer.remain(), self.other.buffer.remain())
+        if is_read:
+          buffer.write(self.other.buffer.read(n))
+        else:
+          self.other.buffer.write(buffer.read(n))
+        self.notify(buffer.progress)
+        self.other.notify(self.other.buffer.progress)
+        if self.other.buffer.remain() == 0:
+          self.other.buffer = None
+      elif buffer.remain() > 0 or (is_read and self.other.buffer.remain() == 0):
+        self.other.notify(progress = 0)
         self.other.buffer = None
-    elif buffer.remain() > 0 or (is_read and self.other.buffer.remain() == 0):
-      self.other.notify(progress = 0)
-      self.other.buffer = None
-      self.buffer = buffer
-    else:
-      self.notify(progress = 0)
+        self.buffer = buffer
+      else:
+        self.notify(progress = 0)
 
   def cancel(self):
     assert(self.state == End.State.COPYING)

design/mvp/canonical-abi/run_tests.py

@@ -1978,7 +1978,7 @@ def core_func1(args):
     result,n = unpack_result(mem1[retp+4])
     assert(n == 4 and result == CopyResult.COMPLETED)
 
-    [ret] = canon_stream_write(StreamType(U8Type()), opts1, wsi, 12345, 0)
+    [ret] = canon_stream_write(StreamType(U8Type()), opts1, wsi, 0, 0)
     assert(ret == definitions.BLOCKED)
 
     fut4.set()
@@ -1988,7 +1988,7 @@ def core_func1(args):
     assert(mem1[retp+0] == wsi)
     assert(mem1[retp+4] == 0)
 
-    [ret] = canon_stream_write(StreamType(U8Type()), opts1, wsi, 12345, 0)
+    [ret] = canon_stream_write(StreamType(U8Type()), opts1, wsi, 0, 0)
     assert(ret == 0)
 
     [errctxi] = canon_error_context_new(opts1, 0, 0)
@@ -2032,7 +2032,7 @@ def core_func2(args):
     fut2.set()
     thread.wait_until(fut3.is_set)
 
-    [ret] = canon_stream_read(StreamType(U8Type()), opts2, rsi, 12345, 0)
+    [ret] = canon_stream_read(StreamType(U8Type()), opts2, rsi, 0, 0)
     assert(ret == 0)
 
     mem2[0:8] = bytes(8)
@@ -2047,7 +2047,7 @@ def core_func2(args):
 
     thread.wait_until(fut4.is_set)
 
-    [ret] = canon_stream_read(StreamType(U8Type()), opts2, rsi, 12345, 0)
+    [ret] = canon_stream_read(StreamType(U8Type()), opts2, rsi, 0, 0)
     assert(ret == definitions.BLOCKED)
 
     [event] = canon_waitable_set_wait(MemInst(mem2, 'i32'), seti, retp)
@@ -2851,12 +2851,74 @@ def core_func(args):
     [] = canon_stream_drop_readable(StreamType(elemt), rsi)
     [] = canon_stream_drop_writable(StreamType(elemt), wsi)
 
+    # A zero-length buffer's pointer is still checked for alignment and bounds
+    # (though it may point at the very end of memory), unless the element type
+    # is empty, in which case the pointer is ignored.
+    [packed] = canon_stream_new(StreamType(elemt))
+    rsi,wsi = unpack_new_ends(packed)
+    if elemt is None:
+      bad_ptrs = []
+      ok_ptr = 0xdeadbeef
+    else:
+      align = alignment(elemt, 'i32')
+      bad_ptrs = [len(mem) + align] + ([1] if align > 1 else [])
+      ok_ptr = len(mem)
+    for ptr in bad_ptrs:
+      expect_fail(lambda: canon_stream_read(StreamType(elemt), async_opts, rsi, ptr, 0))
+      expect_fail(lambda: canon_stream_write(StreamType(elemt), async_opts, wsi, ptr, 0))
+    [ret] = canon_stream_read(StreamType(elemt), async_opts, rsi, ok_ptr, 0)
+    assert(ret == definitions.BLOCKED)
+    [ret] = canon_stream_write(StreamType(elemt), async_opts, wsi, ok_ptr, 0)
+    result,n = unpack_result(ret)
+    assert(n == 0 and result == CopyResult.COMPLETED)
+    [ret] = canon_stream_cancel_read(StreamType(elemt), True, rsi)
+    result,n = unpack_result(ret)
+    assert(n == 0 and result == CopyResult.CANCELLED)
+    [] = canon_stream_drop_readable(StreamType(elemt), rsi)
+    [] = canon_stream_drop_writable(StreamType(elemt), wsi)
+
     [] = canon_waitable_set_drop(seti)
     return []
 
   lift_and_run(sync_opts, inst, ft, core_func, lambda:[], lambda _:())
 
 
+def test_self_copy_trap(elemt):
+  store = Store()
+  inst = ComponentInstance(store)
+  mem = bytearray(40)
+  sync_opts = mk_opts(memory=MemInst(mem, 'i32'), async_=False)
+  async_opts = mk_opts(memory=MemInst(mem, 'i32'), async_=True)
+  future_t = FutureType(elemt)
+  stream_t = StreamType(elemt)
+
+  # Reading and writing the same future or stream from within the same
+  # component instance traps for non-number element types as soon as both ends
+  # have provided a buffer, even if no elements would be copied.
+  ft = FuncType([], [], async_ = True)
+  def core_func(args):
+    [packed] = canon_future_new(future_t)
+    rfi,wfi = unpack_new_ends(packed)
+    [ret] = canon_future_write(future_t, async_opts, wfi, 0)
+    assert(ret == definitions.BLOCKED)
+    expect_fail(lambda: canon_future_read(future_t, async_opts, rfi, 0))
+
+    for write_first in [True, False]:
+      for write_len in [0, 1]:
+        for read_len in [0, 1]:
+          [packed] = canon_stream_new(stream_t)
+          rsi,wsi = unpack_new_ends(packed)
+          write = lambda: canon_stream_write(stream_t, async_opts, wsi, 0, write_len)
+          read = lambda: canon_stream_read(stream_t, async_opts, rsi, 0, read_len)
+          first, second = (write, read) if write_first else (read, write)
+          [ret] = first()
+          assert(ret == definitions.BLOCKED)
+          expect_fail(second)
+    return []
+
+  lift_and_run(sync_opts, inst, ft, core_func, lambda:[], lambda _:())
+
+
 def test_async_flat_params():
   store = Store()
   heap = Heap(1000)
@@ -3148,6 +3210,8 @@ def on_resolve(v):
 test_self_copy(None)
 test_self_copy(U8Type())
 test_self_copy(F64Type())
+test_self_copy_trap(BoolType())
+test_self_copy_trap(StringType())
 test_async_flat_params()
 test_threads()
 test_sync_threads()