Skip to content

Commit 4f8289a

Browse files
Merge pull request from GHSA-3p37-3636-q8wv
in dynarray_make_setter, the length is copied before the data. when the dst and src arrays do not overlap, this is not a problem. however, when the dst and src are the same dynarray, this can lead to a store-before-load, leading any array bounds checks on the right hand side to function incorrectly. here is an example: ```vyper @external def should_revert() -> DynArray[uint256,3]: a: DynArray[uint256, 3] = [1, 2, 3] a = empty(DynArray[uint256, 3]) a = [self.a[0], self.a[1], self.a[2]] return a # if bug: returns [1,2,3] ``` this commit moves the length store to after the data copy in dynarray_make_setter. for hygiene, it also moves the length store to after the data copy in several other routines. I left pop_dyn_array() unchanged, because moving the routine does not actually perform any data copy, it just writes the new length (and optionally returns a pointer to the popped item).
1 parent 92c32f8 commit 4f8289a

File tree

2 files changed

+123
-15
lines changed

2 files changed

+123
-15
lines changed

Diff for: tests/parser/types/test_dynamic_array.py

+92
Original file line numberDiff line numberDiff line change
@@ -1748,3 +1748,95 @@ def foo(i: uint256) -> {return_type}:
17481748
return MY_CONSTANT[i]
17491749
"""
17501750
assert_compile_failed(lambda: get_contract(code), TypeMismatch)
1751+
1752+
1753+
dynarray_length_no_clobber_cases = [
1754+
# GHSA-3p37-3636-q8wv cases
1755+
"""
1756+
a: DynArray[uint256,3]
1757+
1758+
@external
1759+
def should_revert() -> DynArray[uint256,3]:
1760+
self.a = [1,2,3]
1761+
self.a = empty(DynArray[uint256,3])
1762+
self.a = [self.a[0], self.a[1], self.a[2]]
1763+
1764+
return self.a # if bug: returns [1,2,3]
1765+
""",
1766+
"""
1767+
@external
1768+
def should_revert() -> DynArray[uint256,3]:
1769+
self.a()
1770+
return self.b() # if bug: returns [1,2,3]
1771+
1772+
@internal
1773+
def a():
1774+
a: uint256 = 0
1775+
b: uint256 = 1
1776+
c: uint256 = 2
1777+
d: uint256 = 3
1778+
1779+
@internal
1780+
def b() -> DynArray[uint256,3]:
1781+
a: DynArray[uint256,3] = empty(DynArray[uint256,3])
1782+
a = [a[0],a[1],a[2]]
1783+
return a
1784+
""",
1785+
"""
1786+
a: DynArray[uint256,4]
1787+
1788+
@external
1789+
def should_revert() -> DynArray[uint256,4]:
1790+
self.a = [1,2,3]
1791+
self.a = empty(DynArray[uint256,4])
1792+
self.a = [4, self.a[0]]
1793+
1794+
return self.a # if bug: return [4, 4]
1795+
""",
1796+
"""
1797+
@external
1798+
def should_revert() -> DynArray[uint256,4]:
1799+
a: DynArray[uint256, 4] = [1,2,3]
1800+
a = []
1801+
1802+
a = [a.pop()] # if bug: return [1]
1803+
1804+
return a
1805+
""",
1806+
"""
1807+
@external
1808+
def should_revert():
1809+
c: DynArray[uint256, 1] = []
1810+
c.append(c[0])
1811+
""",
1812+
"""
1813+
@external
1814+
def should_revert():
1815+
c: DynArray[uint256, 1] = [1]
1816+
c[0] = c.pop()
1817+
""",
1818+
"""
1819+
@external
1820+
def should_revert():
1821+
c: DynArray[DynArray[uint256, 1], 2] = [[]]
1822+
c[0] = c.pop()
1823+
""",
1824+
"""
1825+
a: DynArray[String[65],2]
1826+
1827+
@external
1828+
def should_revert() -> DynArray[String[65], 2]:
1829+
self.a = ["hello", "world"]
1830+
self.a = []
1831+
self.a = [self.a[0], self.a[1]]
1832+
1833+
return self.a # if bug: return ["hello", "world"]
1834+
""",
1835+
]
1836+
1837+
1838+
@pytest.mark.parametrize("code", dynarray_length_no_clobber_cases)
1839+
def test_dynarray_length_no_clobber(get_contract, assert_tx_failed, code):
1840+
# check that length is not clobbered before dynarray data copy happens
1841+
c = get_contract(code)
1842+
assert_tx_failed(lambda: c.should_revert())

Diff for: vyper/codegen/core.py

+31-15
Original file line numberDiff line numberDiff line change
@@ -117,13 +117,15 @@ def make_byte_array_copier(dst, src):
117117
max_bytes = src.typ.maxlen
118118

119119
ret = ["seq"]
120+
121+
dst_ = bytes_data_ptr(dst)
122+
src_ = bytes_data_ptr(src)
123+
124+
ret.append(copy_bytes(dst_, src_, len_, max_bytes))
125+
120126
# store length
121127
ret.append(STORE(dst, len_))
122128

123-
dst = bytes_data_ptr(dst)
124-
src = bytes_data_ptr(src)
125-
126-
ret.append(copy_bytes(dst, src, len_, max_bytes))
127129
return b1.resolve(b2.resolve(ret))
128130

129131

@@ -148,25 +150,34 @@ def _dynarray_make_setter(dst, src):
148150
if src.value == "~empty":
149151
return IRnode.from_list(STORE(dst, 0))
150152

153+
# copy contents of src dynarray to dst.
154+
# note that in case src and dst refer to the same dynarray,
155+
# in order for get_element_ptr oob checks on the src dynarray
156+
# to work, we need to wait until after the data is copied
157+
# before we clobber the length word.
158+
151159
if src.value == "multi":
152160
ret = ["seq"]
153161
# handle literals
154162

155-
# write the length word
156-
store_length = STORE(dst, len(src.args))
157-
ann = None
158-
if src.annotation is not None:
159-
ann = f"len({src.annotation})"
160-
store_length = IRnode.from_list(store_length, annotation=ann)
161-
ret.append(store_length)
162-
163+
# copy each item
163164
n_items = len(src.args)
165+
164166
for i in range(n_items):
165167
k = IRnode.from_list(i, typ=UINT256_T)
166168
dst_i = get_element_ptr(dst, k, array_bounds_check=False)
167169
src_i = get_element_ptr(src, k, array_bounds_check=False)
168170
ret.append(make_setter(dst_i, src_i))
169171

172+
# write the length word after data is copied
173+
store_length = STORE(dst, n_items)
174+
ann = None
175+
if src.annotation is not None:
176+
ann = f"len({src.annotation})"
177+
store_length = IRnode.from_list(store_length, annotation=ann)
178+
179+
ret.append(store_length)
180+
170181
return ret
171182

172183
with src.cache_when_complex("darray_src") as (b1, src):
@@ -190,8 +201,6 @@ def _dynarray_make_setter(dst, src):
190201
with get_dyn_array_count(src).cache_when_complex("darray_count") as (b2, count):
191202
ret = ["seq"]
192203

193-
ret.append(STORE(dst, count))
194-
195204
if should_loop:
196205
i = IRnode.from_list(_freshname("copy_darray_ix"), typ=UINT256_T)
197206

@@ -213,6 +222,9 @@ def _dynarray_make_setter(dst, src):
213222
dst_ = dynarray_data_ptr(dst)
214223
ret.append(copy_bytes(dst_, src_, n_bytes, max_bytes))
215224

225+
# write the length word after data is copied
226+
ret.append(STORE(dst, count))
227+
216228
return b1.resolve(b2.resolve(ret))
217229

218230

@@ -336,12 +348,14 @@ def append_dyn_array(darray_node, elem_node):
336348
with len_.cache_when_complex("old_darray_len") as (b2, len_):
337349
assertion = ["assert", ["lt", len_, darray_node.typ.count]]
338350
ret.append(IRnode.from_list(assertion, error_msg=f"{darray_node.typ} bounds check"))
339-
ret.append(STORE(darray_node, ["add", len_, 1]))
340351
# NOTE: typechecks elem_node
341352
# NOTE skip array bounds check bc we already asserted len two lines up
342353
ret.append(
343354
make_setter(get_element_ptr(darray_node, len_, array_bounds_check=False), elem_node)
344355
)
356+
357+
# store new length
358+
ret.append(STORE(darray_node, ["add", len_, 1]))
345359
return IRnode.from_list(b1.resolve(b2.resolve(ret)))
346360

347361

@@ -354,6 +368,7 @@ def pop_dyn_array(darray_node, return_popped_item):
354368
new_len = IRnode.from_list(["sub", old_len, 1], typ=UINT256_T)
355369

356370
with new_len.cache_when_complex("new_len") as (b2, new_len):
371+
# store new length
357372
ret.append(STORE(darray_node, new_len))
358373

359374
# NOTE skip array bounds check bc we already asserted len two lines up
@@ -364,6 +379,7 @@ def pop_dyn_array(darray_node, return_popped_item):
364379
location = popped_item.location
365380
else:
366381
typ, location = None, None
382+
367383
return IRnode.from_list(b1.resolve(b2.resolve(ret)), typ=typ, location=location)
368384

369385

0 commit comments

Comments
 (0)