diff --git a/.release-notes/4174.md b/.release-notes/4174.md new file mode 100644 index 0000000000..30a361f8ce --- /dev/null +++ b/.release-notes/4174.md @@ -0,0 +1,23 @@ +## Fix for potential memory corruption in `Array.copy_to` + +`Array.copy_to` did not check whether the source or destination Arrays had been initialized or whether the requested number of elements to be copied exceeded the number of available elements (allocated memory). These issues would result in potential dereferencing of a null pointer or attempts to access unallocated memory. + +Before this fix, the following code would print `11` then `0`: + +```pony +actor Main + new create(e: Env) => + var src: Array[U8] = [1] + var dest: Array[U8] = [11; 1; 2; 3; 4; 5; 6] + + try + e.out.print(dest(0)?.string()) + end + src.copy_to(dest, 11, 0, 10) + + try + e.out.print(dest(0)?.string()) + end +``` + +After the fix, this code correctly prints `11` and `11`. diff --git a/packages/builtin/array.pony b/packages/builtin/array.pony index 0405e3210d..c1ebcf3ab8 100644 --- a/packages/builtin/array.pony +++ b/packages/builtin/array.pony @@ -552,11 +552,16 @@ class Array[A] is Seq[A] """ Copy len elements from this(src_idx) to dst(dst_idx). """ - dst.reserve(dst_idx + len) - _ptr._offset(src_idx)._copy_to(dst._ptr._offset(dst_idx), len) - - if dst._size < (dst_idx + len) then - dst._size = dst_idx + len + if (src_idx < _size) and (dst_idx <= dst._size) then + let count = len.min(_size - src_idx) + if count > 0 then + dst.reserve(dst_idx + count) + _ptr._offset(src_idx)._copy_to(dst._ptr._offset(dst_idx), count) + + if dst._size < (dst_idx + count) then + dst._size = dst_idx + count + end + end end fun ref remove(i: USize, n: USize) => diff --git a/packages/builtin_test/_test.pony b/packages/builtin_test/_test.pony index 2d603c80c9..9879b44f25 100644 --- a/packages/builtin_test/_test.pony +++ b/packages/builtin_test/_test.pony @@ -30,6 +30,7 @@ actor \nodoc\ Main is TestList test(_TestArrayConcat) test(_TestArrayFind) test(_TestArrayFromCPointer) + test(_TestArrayCopyTo) test(_TestArrayInsert) test(_TestArraySlice) test(_TestArraySwapElements) @@ -1764,6 +1765,87 @@ class \nodoc\ iso _TestArrayFromCPointer is UnitTest let arr = Array[U8].from_cpointer(Pointer[U8], 1, 1) h.assert_eq[USize](0, arr.size()) +class \nodoc\ iso _TestArrayCopyTo is UnitTest + fun name(): String => + "builtin/Array.copy_to" + + fun apply(h: TestHelper) => + // Test that a using an uninitialized array as a source leaves the destination unchanged + let src1: Array[U8] = [] + let dest1: Array[U8] = [0; 1; 2; 3; 4; 5; 6] + src1.copy_to(dest1, 0, 0, 10) + h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest1) + + // Test that copying from an empty source array leaves + // the destination unchanged + let src2: Array[U8] = [1] + let dest2: Array[U8] = [0; 1; 2; 3; 4; 5; 6] + try src2.pop()? end + src2.copy_to(dest2, 0, 0, 10) + h.assert_eq[USize](0, src2.size()) + h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest2) + + // try to copy 10 elements; some non-existant + let src3: Array[U8] = [1] + let dest3: Array[U8] = [0; 1; 2; 3; 4; 5; 6] + src3.copy_to(dest3, 0, 0, 10) + h.assert_array_eq[U8]([1; 1; 2; 3; 4; 5; 6], dest3) + + // try to copy from too high start index + let src4: Array[U8] = [1] + let dest4: Array[U8] = [0; 1; 2; 3; 4; 5; 6] + src4.copy_to(dest4, 11, 0, 1) + h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest4) + + // copies the sole available element + let src5: Array[U8] = [1] + let dest5: Array[U8] = [0; 1; 2; 3; 4; 5; 6] + src5.copy_to(dest5, 0, 0, 10) + h.assert_array_eq[U8]([1; 1; 2; 3; 4; 5; 6], dest5) + + // copy larger than destination chunk into the middle of destination + let src6: Array[U8] = [0; 1; 2; 3; 4; 5; 6] + let dest6: Array[U8] = [7; 8; 9] + src6.copy_to(dest6, 0, 1, 7) + h.assert_array_eq[U8]([7; 0; 1; 2; 3; 4; 5; 6], dest6) + + // copy into the front of destination so copied amount fits within + // existing destination + let src7: Array[U8] = [0; 1] + let dest7: Array[U8] = [4; 5; 6] + src7.copy_to(dest7, 0, 0, 2) + h.assert_array_eq[U8]([0; 1; 6], dest7) + + // copy into the middle of a destination + let src8: Array[U8] = [0] + let dest8: Array[U8] = [4; 5; 6] + src8.copy_to(dest8, 0, 1, 1) + h.assert_array_eq[U8]([4; 0; 6], dest8) + + // copy onto the end of the destination + let src9: Array[U8] = [0] + let dest9: Array[U8] = [4; 5; 6] + src9.copy_to(dest9, 0, 2, 1) + h.assert_array_eq[U8]([4; 5; 0], dest9) + + // destination is empty + let src10: Array[U8] = [0; 1; 2; 3] + let dest10: Array[U8] = [] + src10.copy_to(dest10, 0, 0, 4) + h.assert_array_eq[U8]([0; 1; 2; 3], dest10) + + // copy right after the end of the destination + let src11: Array[U8] = [0] + let dest11: Array[U8] = [4; 5; 6] + src11.copy_to(dest11, 0, 3, 1) + h.assert_array_eq[U8]([4; 5; 6; 0], dest11) + + // destination index is "someone else's memory" + let src12: Array[U8] = [0] + let dest12: Array[U8] = [4; 5; 6] + src12.copy_to(dest12, 0, 100, 1) + h.assert_array_eq[U8]([4; 5; 6], dest12) + class \nodoc\ iso _TestMath128 is UnitTest """ Test 128 bit integer math. diff --git a/test/full-program-tests/regression-4174/expected-exit-code.txt b/test/full-program-tests/regression-4174/expected-exit-code.txt new file mode 100644 index 0000000000..b4de394767 --- /dev/null +++ b/test/full-program-tests/regression-4174/expected-exit-code.txt @@ -0,0 +1 @@ +11 diff --git a/test/full-program-tests/regression-4174/main.pony b/test/full-program-tests/regression-4174/main.pony new file mode 100644 index 0000000000..baabefdd03 --- /dev/null +++ b/test/full-program-tests/regression-4174/main.pony @@ -0,0 +1,11 @@ +use @pony_exitcode[None](code: I32) + +actor Main + new create(e: Env) => + var src: Array[U8] = [1] + var dest: Array[U8] = [11; 1; 2; 3; 4; 5; 6] + src.copy_to(dest, 11, 0, 10) + try + let v = dest(0)?.i32() + @pony_exitcode(v) + end