ponyc: Memory corruption via Array.copy_to

There’s bugs in Array’s copy to where it doesn’t validate input

for example:

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

prints 11 and then prints 0. copying from an out of index src_index shouldn’t be changing the destination array, it should be an error.

To fix, copy_to needs to become partial.

As part of this change, the slice method that use copy_to be switched to using _copy_to so that it isn’t required to become partial UNLESS, slice actually needs to be partial. We should validate that it doesn’t.

Some things that we should be adding copy_to tests methods for:

  • src index is outside of range (should be a bug now)
  • dest index is outside of valid range (should be handled correctly now)
  • length to copy goes outside of valid range (should be a bug now)
  • array was never initialized (so _ptr is null) (see example-ish test in #4173)
  • array was initialized but is now empty (_ptr isn’t null, but size is 0) (see example-ish test in #4173)

Here’s some example code that shows the length bug:

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(3)?.string())
    end
    src.copy_to(dest, 0, 0, 10)
    try
      e.out.print(dest(3)?.string())
    end

About this issue

  • Original URL
  • State: closed
  • Created 2 years ago
  • Comments: 21 (21 by maintainers)

Commits related to this issue

Most upvoted comments

@jemc Your proposal sounds good. I came round on your idea: your point about having a silent breaking change was a good one.