From 266986a2ffb826585e1cbe52d3fc6b955377e8a2 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Mon, 26 Feb 2024 11:10:41 -0500 Subject: [PATCH 1/8] fix[codegen]: bounds check for signed index accesses --- vyper/codegen/core.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/vyper/codegen/core.py b/vyper/codegen/core.py index 8a186fe683..d96f117926 100644 --- a/vyper/codegen/core.py +++ b/vyper/codegen/core.py @@ -567,12 +567,10 @@ def _get_element_ptr_array(parent, key, array_bounds_check): if array_bounds_check: is_darray = isinstance(parent.typ, DArrayT) bound = get_dyn_array_count(parent) if is_darray else parent.typ.count - # uclamplt works, even for signed ints. since two's-complement - # is used, if the index is negative, (unsigned) LT will interpret - # it as a very large number, larger than any practical value for - # an array index, and the clamp will throw an error. - # NOTE: there are optimization rules for this when ix or bound is literal - ix = clamp("lt", ix, bound) + # NOTE: there are optimization rules for the bounds check when + # ix or bound is literal. there is also an optimization rule to + # optimize out ix>=0 when ix is unsigned. + ix = clamp2(ix, 0, bound) ix.set_error_msg(f"{parent.typ} bounds check") if parent.encoding == Encoding.ABI: From b70777f483413c3e50d24c4d4401ca11973e5b3b Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Mon, 26 Feb 2024 11:36:00 -0500 Subject: [PATCH 2/8] fix bad argument --- vyper/codegen/core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vyper/codegen/core.py b/vyper/codegen/core.py index d96f117926..c5964cd4e9 100644 --- a/vyper/codegen/core.py +++ b/vyper/codegen/core.py @@ -570,7 +570,7 @@ def _get_element_ptr_array(parent, key, array_bounds_check): # NOTE: there are optimization rules for the bounds check when # ix or bound is literal. there is also an optimization rule to # optimize out ix>=0 when ix is unsigned. - ix = clamp2(ix, 0, bound) + ix = clamp2(ix, 0, bound, ix.typ.is_signed) ix.set_error_msg(f"{parent.typ} bounds check") if parent.encoding == Encoding.ABI: From f41ef420d0b6872dd082342f4c7b0d09a63812bd Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Mon, 26 Feb 2024 14:57:24 -0500 Subject: [PATCH 3/8] fix clamp2 argument order --- vyper/codegen/core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vyper/codegen/core.py b/vyper/codegen/core.py index c5964cd4e9..a7b4794b57 100644 --- a/vyper/codegen/core.py +++ b/vyper/codegen/core.py @@ -570,7 +570,7 @@ def _get_element_ptr_array(parent, key, array_bounds_check): # NOTE: there are optimization rules for the bounds check when # ix or bound is literal. there is also an optimization rule to # optimize out ix>=0 when ix is unsigned. - ix = clamp2(ix, 0, bound, ix.typ.is_signed) + ix = clamp2(0, ix, bound, ix.typ.is_signed) ix.set_error_msg(f"{parent.typ} bounds check") if parent.encoding == Encoding.ABI: From 21b80205eefa3ca7feabbe5c6f79722dfb5077d7 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Mon, 26 Feb 2024 15:11:32 -0500 Subject: [PATCH 4/8] fix oob checks --- vyper/codegen/core.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/vyper/codegen/core.py b/vyper/codegen/core.py index a7b4794b57..2dbc45117f 100644 --- a/vyper/codegen/core.py +++ b/vyper/codegen/core.py @@ -568,9 +568,15 @@ def _get_element_ptr_array(parent, key, array_bounds_check): is_darray = isinstance(parent.typ, DArrayT) bound = get_dyn_array_count(parent) if is_darray else parent.typ.count # NOTE: there are optimization rules for the bounds check when - # ix or bound is literal. there is also an optimization rule to - # optimize out ix>=0 when ix is unsigned. - ix = clamp2(0, ix, bound, ix.typ.is_signed) + # ix or bound is literal + with ix.cache_when_complex("ix") as (b1, ix): + LT = "slt" if ix.typ.is_signed else "lt" + # note: this is optimized out for unsigned integers + is_negative = [LT, ix, 0] + # always use unsigned gt, since bound is always an unsigned quantity + is_oob = ["gt", ix, bound] + checked_ix = ["seq", ["assert", ["iszero", ["or", is_negative, is_oob]]], ix] + ix = b1.resolve(IRnode.from_list(checked_ix)) ix.set_error_msg(f"{parent.typ} bounds check") if parent.encoding == Encoding.ABI: From 105ea782195466239078cf84e4d3241f7fc8def4 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Mon, 26 Feb 2024 15:14:46 -0500 Subject: [PATCH 5/8] fix an off-by-one --- vyper/codegen/core.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/vyper/codegen/core.py b/vyper/codegen/core.py index 2dbc45117f..8c4295e5cc 100644 --- a/vyper/codegen/core.py +++ b/vyper/codegen/core.py @@ -573,8 +573,8 @@ def _get_element_ptr_array(parent, key, array_bounds_check): LT = "slt" if ix.typ.is_signed else "lt" # note: this is optimized out for unsigned integers is_negative = [LT, ix, 0] - # always use unsigned gt, since bound is always an unsigned quantity - is_oob = ["gt", ix, bound] + # always use unsigned ge, since bound is always an unsigned quantity + is_oob = ["ge", ix, bound] checked_ix = ["seq", ["assert", ["iszero", ["or", is_negative, is_oob]]], ix] ix = b1.resolve(IRnode.from_list(checked_ix)) ix.set_error_msg(f"{parent.typ} bounds check") From 72f6018c02e3567bacff7062c736faa767ae5a99 Mon Sep 17 00:00:00 2001 From: cyberthirst Date: Tue, 27 Feb 2024 10:15:40 +0100 Subject: [PATCH 6/8] chore: add array indexing tests --- .../codegen/types/test_array_indexing.py | 130 ++++++++++++++++++ 1 file changed, 130 insertions(+) create mode 100644 tests/functional/codegen/types/test_array_indexing.py diff --git a/tests/functional/codegen/types/test_array_indexing.py b/tests/functional/codegen/types/test_array_indexing.py new file mode 100644 index 0000000000..8341acb62c --- /dev/null +++ b/tests/functional/codegen/types/test_array_indexing.py @@ -0,0 +1,130 @@ +import pytest + +from eth_tester.exceptions import TransactionFailed + +# TODO: rewrite the tests in type-centric way, parametrize array and indices types + + +def test_negative_ix_access(get_contract): + # Arrays can't be accessed with negative indices + code = """ +arr: uint256[3] + +@external +def foo(i: int128): + self.arr[i] = 1 + """ + + c = get_contract(code) + + with pytest.raises(TransactionFailed): + c.foo(-1) + c.foo(-3) + c.foo(-2**127+1) + + +def test_negative_ix_access_to_large_arr(get_contract): + # Arrays can't be accessed with negative indices + code = """ +arr: public(uint256[max_value(uint256)-1]) + +@external +def set(idx: int256): + self.arr[idx] = 1 + """ + + c = get_contract(code) + with pytest.raises(TransactionFailed): + c.set(-2**255) + c.set(-2**255+5) + c.set(-2**128) + c.set(-1) + + +def test_oob_access_to_large_arr(get_contract): + # Test OOB access to large array + code = """ +arr: public(uint256[max_value(uint256)-1]) + +@external +def set(idx: int256): + self.arr[idx] = 3 + +@external +def set2(idx: uint256): + self.arr[idx] = 3 + """ + c = get_contract(code) + + with pytest.raises(TransactionFailed): + c.set2(2**256-1, transact={}) + c.set2(2**256-2, transact={}) + + +def test_boundary_access_to_arr(get_contract): + # Test access to the boundary of the array + code = """ +arr1: public(int256[max_value(int256)]) + +@external +def set1(idx: int256): + self.arr1[idx] = 3 + + """ + code2 = """ +arr2: public(uint256[max_value(uint256)-1]) + +@external +def set2(idx: uint256): + self.arr2[idx] = 3 + """ + c1 = get_contract(code) + + c1.set1(2 ** 255 - 2, transact={}) + assert c1.arr1(2 ** 255 - 2) == 3 + c1.set1(0, transact={}) + assert c1.arr1(0) == 3 + + c2 = get_contract(code2) + + c2.set2(2 ** 256 - 3, transact={}) + assert c2.arr2(2 ** 256 - 3) == 3 + + +def test_valid_ix_access(get_contract): + code = """ +arr: public(uint256[3]) +arr2: public(int256[3]) + +@external +def foo(i: int128): + self.arr[i] = 1 + +@external +def bar(i: uint256): + self.arr[i] = 2 + """ + + c = get_contract(code) + for i in range(3): + c.foo(i, transact={}) + assert c.arr(i) == 1 + c.bar(i, transact={}) + assert c.arr(i) == 2 + + +def test_for_loop_ix_access(get_contract): + # Arrays can be accessed with for loop iterators of type int + code = """ +arr: public(int256[10]) + +@external +def foo(): + for i: int256 in range(10): + self.arr[i] = i + """ + + c = get_contract(code) + c.foo(transact={}) + for i in range(10): + assert c.arr(i) == i \ No newline at end of file From beecc5aa82b760a4677363d4e2a906309874076c Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Tue, 27 Feb 2024 07:36:29 -0500 Subject: [PATCH 7/8] fix lint --- .../codegen/types/test_array_indexing.py | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/tests/functional/codegen/types/test_array_indexing.py b/tests/functional/codegen/types/test_array_indexing.py index 8341acb62c..f51a77297e 100644 --- a/tests/functional/codegen/types/test_array_indexing.py +++ b/tests/functional/codegen/types/test_array_indexing.py @@ -20,7 +20,7 @@ def foo(i: int128): with pytest.raises(TransactionFailed): c.foo(-1) c.foo(-3) - c.foo(-2**127+1) + c.foo(-(2**127) + 1) def test_negative_ix_access_to_large_arr(get_contract): @@ -35,9 +35,9 @@ def set(idx: int256): c = get_contract(code) with pytest.raises(TransactionFailed): - c.set(-2**255) - c.set(-2**255+5) - c.set(-2**128) + c.set(-(2**255)) + c.set(-(2**255) + 5) + c.set(-(2**128)) c.set(-1) @@ -49,7 +49,7 @@ def test_oob_access_to_large_arr(get_contract): @external def set(idx: int256): self.arr[idx] = 3 - + @external def set2(idx: uint256): self.arr[idx] = 3 @@ -57,8 +57,8 @@ def set2(idx: uint256): c = get_contract(code) with pytest.raises(TransactionFailed): - c.set2(2**256-1, transact={}) - c.set2(2**256-2, transact={}) + c.set2(2**256 - 1, transact={}) + c.set2(2**256 - 2, transact={}) def test_boundary_access_to_arr(get_contract): @@ -80,15 +80,15 @@ def set2(idx: uint256): """ c1 = get_contract(code) - c1.set1(2 ** 255 - 2, transact={}) - assert c1.arr1(2 ** 255 - 2) == 3 + c1.set1(2**255 - 2, transact={}) + assert c1.arr1(2**255 - 2) == 3 c1.set1(0, transact={}) assert c1.arr1(0) == 3 c2 = get_contract(code2) - c2.set2(2 ** 256 - 3, transact={}) - assert c2.arr2(2 ** 256 - 3) == 3 + c2.set2(2**256 - 3, transact={}) + assert c2.arr2(2**256 - 3) == 3 def test_valid_ix_access(get_contract): @@ -99,7 +99,7 @@ def test_valid_ix_access(get_contract): @external def foo(i: int128): self.arr[i] = 1 - + @external def bar(i: uint256): self.arr[i] = 2 @@ -127,4 +127,4 @@ def foo(): c = get_contract(code) c.foo(transact={}) for i in range(10): - assert c.arr(i) == i \ No newline at end of file + assert c.arr(i) == i From 635a3d791c5fa90a3b8eebdbf80c419a0a736f9d Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Tue, 27 Feb 2024 07:55:54 -0500 Subject: [PATCH 8/8] fix lint again --- tests/functional/codegen/types/test_array_indexing.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/functional/codegen/types/test_array_indexing.py b/tests/functional/codegen/types/test_array_indexing.py index f51a77297e..321f6e620c 100644 --- a/tests/functional/codegen/types/test_array_indexing.py +++ b/tests/functional/codegen/types/test_array_indexing.py @@ -1,5 +1,4 @@ import pytest - from eth_tester.exceptions import TransactionFailed # TODO: rewrite the tests in type-centric way, parametrize array and indices types