Skip to content

Commit

Permalink
Fix memory corruption that can arise from Array.copy_to
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 committed Feb 4, 2024
1 parent 17ee1a5 commit 7448a09
Show file tree
Hide file tree
Showing 3 changed files with 70 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
37 changes: 37 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,42 @@ 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]
try src2.pop()? end
h.assert_eq[USize](0, src2.size())

let src3: Array[U8] = [1]
let dest3: Array[U8] = [0; 1; 2; 3; 4; 5; 6]
// try to copy 10 non-existant elements
src3.copy_to(dest3, 0, 0, 10)
h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest3)


let src4: Array[U8] = [1]
let dest4: Array[U8] = [0; 1; 2; 3; 4; 5; 6]
// try to copy from too high start index
src4.copy_to(dest4, 11, 0, 1)
h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest4)

let src5: Array[U8] = [1]
let dest5: Array[U8] = [0; 1; 2; 3; 4; 5; 6]
// copies the sole available element
src5.copy_to(dest5, 0, 0, 10)
h.assert_array_eq[U8]([1; 1; 2; 3; 4; 5; 6], dest5)

class \nodoc\ iso _TestMath128 is UnitTest
"""
Test 128 bit integer math.
Expand Down

0 comments on commit 7448a09

Please sign in to comment.