fix[codegen]: fix bytes copying routines for 0-length case (#4649)

per title. as in the referenced pull requests and security advisories,
it is difficult, but not impossible, to construct an empty bytestring
which has side effects. in this commit, don't fastpath out the side
effects for zero-length bytestrings

references:
- #4644
- GHSA-qhr6-mgqr-mchm
- #4645
- GHSA-3vcg-j39x-cwfm

---------

Co-authored-by: cyberthirst <[email protected]>

Author: charles-cooper

tests/functional/builtins/codegen/test_empty.py

@@ -1,6 +1,7 @@
 import pytest
 
-from vyper.exceptions import InstantiationException, TypeMismatch
+from vyper.compiler import compile_code
+from vyper.exceptions import ArrayIndexException, InstantiationException, TypeMismatch
 
 
 @pytest.mark.parametrize(
@@ -692,14 +693,39 @@ def foo():
 
 
 @pytest.mark.parametrize(
-    "contract",
+    "code, exc",
     [
-        """
+        (
+            """
 @external
 def test():
     a: uint256 = empty(HashMap[uint256, uint256])[0]
-    """
+    """,
+            InstantiationException,
+        ),
+        (
+            """
+@external
+def test():
+    a: Bytes[32] = empty(Bytes[0])
+    """,
+            ArrayIndexException,
+        ),
     ],
 )
-def test_invalid_types(contract, get_contract, assert_compile_failed):
-    assert_compile_failed(lambda: get_contract(contract), InstantiationException)
+def test_invalid_types(code, exc):
+    with pytest.raises(exc):
+        compile_code(code)
+
+
+@pytest.mark.parametrize("empty_bytes", ["x''", "b''"])
+@pytest.mark.parametrize("size", [1] + [i for i in range(1 * 32, 5 * 32, 32)])
+def test_empty_Bytes(get_contract, size, empty_bytes):
+    code = f"""
+@external
+def foo() -> bool:
+    b: Bytes[{size}] = empty(Bytes[{size}])
+    return b == {empty_bytes}
+"""
+    c = get_contract(code)
+    assert c.foo() is True

tests/functional/builtins/codegen/test_slice.py

@@ -633,3 +633,37 @@ def foo() -> Bytes[96]:
 
     c = get_contract(slice_code)
     assert c.foo() == b"defghijklmnopqrstuvwxyz123456789"
+
+
+def test_slice_empty_bytes32(get_contract):
+    code = """
+@external
+def bar() -> Bytes[32]:
+    return slice(empty(bytes32), 0, 32)
+    """
+    c = get_contract(code)
+    assert c.bar() == b"\x00" * 32
+
+
+def test_slice_empty_Bytes32_0(get_contract, tx_failed):
+    code = """
+@external
+def bar(length: uint256) -> Bytes[32]:
+    # use variable length otherwise it gets optimized to
+    # StaticAssertionException
+    return slice(empty(Bytes[32]), 0, length)
+    """
+    c = get_contract(code)
+    with tx_failed():
+        _ = c.bar(1)
+
+
+def test_slice_empty_Bytes32_1(get_contract):
+    code = """
+@external
+def bar() -> Bytes[32]:
+    length: uint256 = 0
+    return slice(empty(Bytes[32]), 0, length)
+    """
+    c = get_contract(code)
+    assert c.bar() == b""

vyper/builtins/functions.py

@@ -356,7 +356,7 @@ def build_IR(self, expr, args, kwargs, context):
         if src.location is None:
             # it's not a pointer; force it to be one since
             # copy_bytes works on pointers.
-            assert is_bytes32, src
+            assert is_bytes32 or src.is_empty_intrinsic, src
             src = ensure_in_memory(src, context)
         elif potential_overlap(src, start) or potential_overlap(src, length):
             src = create_memory_copy(src, context)

vyper/codegen/core.py

@@ -185,12 +185,12 @@ def make_byte_array_copier(dst, src):
 
     _check_assign_bytes(dst, src)
 
-    # TODO: remove this branch, copy_bytes and get_bytearray_length should handle
-    if src.value == "~empty" or src.typ.maxlen == 0:
-        # set length word to 0.
-        return STORE(dst, 0)
-
     with src.cache_when_complex("src") as (b1, src):
+        if src.typ.maxlen == 0 or src.is_empty_intrinsic:
+            # set dst length to zero, preserving side effects of `src`.
+            ret = STORE(dst, 0)
+            return b1.resolve(ret)
+
         if src.typ.maxlen <= 32 and not copy_opcode_available(dst, src):
             # if there is no batch copy opcode available,
             # it's cheaper to run two load/stores instead of copy_bytes
@@ -281,7 +281,7 @@ def _dynarray_make_setter(dst, src, hi=None):
     assert isinstance(src.typ, DArrayT)
     assert isinstance(dst.typ, DArrayT)
 
-    if src.value == "~empty":
+    if src.is_empty_intrinsic:
         return IRnode.from_list(STORE(dst, 0))
 
     # copy contents of src dynarray to dst.
@@ -390,10 +390,12 @@ def copy_bytes(dst, src, length, length_bound):
 
         # correctness: do not clobber dst
         if length_bound == 0:
-            return IRnode.from_list(["seq"], annotation=annotation)
+            ret = IRnode.from_list(["seq"], annotation=annotation)
+            return b1.resolve(b2.resolve(b3.resolve(ret)))
         # performance: if we know that length is 0, do not copy anything
         if length.value == 0:
-            return IRnode.from_list(["seq"], annotation=annotation)
+            ret = IRnode.from_list(["seq"], annotation=annotation)
+            return b1.resolve(b2.resolve(b3.resolve(ret)))
 
         assert src.is_pointer and dst.is_pointer
 
@@ -457,7 +459,7 @@ def get_bytearray_length(arg):
 
     # TODO: it would be nice to merge the implementations of get_bytearray_length and
     # get_dynarray_count
-    if arg.value == "~empty":
+    if arg.is_empty_intrinsic:
         return IRnode.from_list(0, typ=typ)
 
     return IRnode.from_list(LOAD(arg), typ=typ)
@@ -472,7 +474,7 @@ def get_dyn_array_count(arg):
     if arg.value == "multi":
         return IRnode.from_list(len(arg.args), typ=typ)
 
-    if arg.value == "~empty":
+    if arg.is_empty_intrinsic:
         # empty(DynArray[...])
         return IRnode.from_list(0, typ=typ)
 
@@ -586,7 +588,7 @@ def _get_element_ptr_tuplelike(parent, key, hi=None):
         annotation = None
 
     # generated by empty() + make_setter
-    if parent.value == "~empty":
+    if parent.is_empty_intrinsic:
         return IRnode.from_list("~empty", typ=subtype)
 
     if parent.value == "multi":
@@ -635,7 +637,7 @@ def _get_element_ptr_array(parent, key, array_bounds_check):
 
     subtype = parent.typ.value_type
 
-    if parent.value == "~empty":
+    if parent.is_empty_intrinsic:
         if array_bounds_check:
             # this case was previously missing a bounds check. codegen
             # is a bit complicated when bounds check is required, so
@@ -765,7 +767,7 @@ def unwrap_location(orig):
         return IRnode.from_list(LOAD(orig), typ=orig.typ)
     else:
         # CMC 2022-03-24 TODO refactor so this branch can be removed
-        if orig.value == "~empty":
+        if orig.is_empty_intrinsic:
             # must be word type
             return IRnode.from_list(0, typ=orig.typ)
         return orig
@@ -821,13 +823,14 @@ def dummy_node_for_type(typ):
     return IRnode("fake_node", typ=typ)
 
 
-def _check_assign_bytes(left, right):
-    if right.typ.maxlen > left.typ.maxlen:  # pragma: nocover
+def _check_assign_bytes(left, right):  # pragma: nocover
+    if right.typ.maxlen > left.typ.maxlen:
         raise TypeMismatch(f"Cannot cast from {right.typ} to {left.typ}")
 
     # stricter check for zeroing a byte array.
     # TODO: these should be TypeCheckFailure instead of TypeMismatch
-    if right.value == "~empty" and right.typ.maxlen != left.typ.maxlen:  # pragma: nocover
+    rlen = right.typ.maxlen
+    if right.is_empty_intrinsic and rlen != 0 and rlen != left.typ.maxlen:
         raise TypeMismatch(f"Cannot cast from empty({right.typ}) to {left.typ}")
 
 
@@ -858,7 +861,7 @@ def FAIL():  # pragma: no cover
             FAIL()
 
         # stricter check for zeroing
-        if right.value == "~empty" and right.typ.count != left.typ.count:  # pragma: nocover
+        if right.is_empty_intrinsic and right.typ.count != left.typ.count:  # pragma: nocover
             raise TypeCheckFailure(
                 f"Bad type for clearing bytes: expected {left.typ} but got {right.typ}"
             )
@@ -1109,7 +1112,7 @@ def copy_opcode_available(left, right):
 
 
 def _complex_make_setter(left, right, hi=None):
-    if right.value == "~empty" and left.location == MEMORY:
+    if right.is_empty_intrinsic and left.location == MEMORY:
         # optimized memzero
         return mzero(left, left.typ.memory_bytes_required)
 

vyper/codegen/expr.py

@@ -146,6 +146,11 @@ def parse_HexBytes(self):
     def _make_bytelike(cls, context, typeclass, bytez):
         bytez_length = len(bytez)
         btype = typeclass(bytez_length)
+
+        if bytez_length == 0:
+            # optimization: handled specially by make_byte_array_copier
+            return IRnode.from_list("~empty", typ=btype, annotation=f"empty {btype}")
+
         placeholder = context.new_internal_variable(btype)
         seq = []
         seq.append(["mstore", placeholder, bytez_length])
@@ -157,6 +162,7 @@ def _make_bytelike(cls, context, typeclass, bytez):
                     bytes_to_int((bytez + b"\x00" * 31)[i : i + 32]),
                 ]
             )
+
         return IRnode.from_list(
             ["seq"] + seq + [placeholder],
             typ=btype,

vyper/codegen/ir_node.py

@@ -363,6 +363,14 @@ def __deepcopy__(self, memo):
     def gas(self):
         return self._gas + self.add_gas_estimate
 
+    @property
+    def is_empty_intrinsic(self):
+        if self.value == "~empty":
+            return True
+        if self.value == "seq":
+            return len(self.args) == 1 and self.args[0].is_empty_intrinsic
+        return False
+
     # the IR should be cached and/or evaluated exactly once
     @property
     def is_complex_ir(self):
@@ -376,6 +384,7 @@ def is_complex_ir(self):
             isinstance(self.value, str)
             and (self.value.lower() in VALID_IR_MACROS or self.value.upper() in get_ir_opcodes())
             and self.value.lower() not in do_not_cache
+            and not self.is_empty_intrinsic
         )
 
     # set an error message and push down to its children that don't have error_msg set