Skip to content

Commit

Permalink
Fix memory corruption that can arise from Array.copy_to (#4490)
Browse files Browse the repository at this point in the history
This is based heavily on stefandd's work from
#4173

Closes #4174
  • Loading branch information
SeanTAllen authored Feb 6, 2024
1 parent 4522447 commit 0d5ff2b
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 5 deletions.
23 changes: 23 additions & 0 deletions .release-notes/4174.md
Original file line number Diff line number Diff line change
@@ -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`.
15 changes: 10 additions & 5 deletions packages/builtin/array.pony
Original file line number Diff line number Diff line change
Expand Up @@ -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) =>
Expand Down
82 changes: 82 additions & 0 deletions packages/builtin_test/_test.pony
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ actor \nodoc\ Main is TestList
test(_TestArrayConcat)
test(_TestArrayFind)
test(_TestArrayFromCPointer)
test(_TestArrayCopyTo)
test(_TestArrayInsert)
test(_TestArraySlice)
test(_TestArraySwapElements)
Expand Down Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
11
11 changes: 11 additions & 0 deletions test/full-program-tests/regression-4174/main.pony
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 0d5ff2b

Please sign in to comment.