Attempt to fix potential compiler segfault in lookup.c

.release-notes/4173.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.
+
+```pony
+class Foo
+  embed _array: Array[I32] = []
+
+  new create() => 
+    None
+
+  new clone(src: Foo) =>
+    src._array.copy_to(_array, 0, 0, 10)
+
+actor Main
+  new create(env': Env) =>
+  let foo1 = Foo
+  let foo2 = Foo.clone(foo1)
+```
+
+`Array` is part of the `builtin` package of the standard library. Only code in `builtin` is allowed to do direct pointer manipulations like those that `Array` uses to implement `copy_to`. Direct pointer manipulations, if done incorrectly, can lead to segfaults and other memory related errors.
+
+We consider unsafe pointer operations in the `builtin` package to be of utmost importance and once this bug was found moved quickly to release a fix.

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) =>

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,31 @@ 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 dest2: Array[U8] = [0; 1; 2; 3; 4; 5; 6]
+    src2.copy_to(dest2, 0, 0, 10) // try to copy 10 non-existant elements
+    h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest2)
+    src2.push(1) // re-add single element [1]
+    src2.copy_to(dest2, 11, 0, 1) // try to copy from too high start index
+    h.assert_array_eq[U8]([0; 1; 2; 3; 4; 5; 6], dest2)
+    src2.copy_to(dest2, 0, 0, 10) // copies the sole available element
+    h.assert_array_eq[U8]([1; 1; 2; 3; 4; 5; 6], dest2)
+
 class \nodoc\ iso _TestMath128 is UnitTest
   """
   Test 128 bit integer math.

src/libponyc/codegen/genmatch.c

@@ -62,7 +62,7 @@ static ast_t* eq_param_type(compile_t* c, ast_t* pattern)
 {
   ast_t* pattern_type = deferred_reify(c->frame->reify, ast_type(pattern),
     c->opt);
-  deferred_reification_t* fun = lookup(NULL, pattern, pattern_type, c->str_eq);
+  deferred_reification_t* fun = lookup(c->opt, pattern, pattern_type, c->str_eq);
 
   AST_GET_CHILDREN(fun->ast, cap, id, typeparams, params, result, partial);
   ast_t* param = ast_child(params);

src/libponyc/reach/reach.c

@@ -141,7 +141,7 @@ static reach_method_t* reach_rmethod(reach_method_name_t* n, const char* name)
 }
 
 static reach_method_name_t* add_method_name(reach_type_t* t, const char* name,
-  bool internal)
+  pass_opt_t* opt, bool internal)
 {
   reach_method_name_t* n = reach_method_name(t, name);
 
@@ -159,7 +159,7 @@ static reach_method_name_t* add_method_name(reach_type_t* t, const char* name,
       n->cap = TK_BOX;
       n->internal = true;
     } else {
-      deferred_reification_t* fun = lookup(NULL, NULL, t->ast, name);
+      deferred_reification_t* fun = lookup(opt, NULL, t->ast, name);
       ast_t* fun_ast = fun->ast;
       n->id = ast_id(fun_ast);
       n->cap = ast_id(ast_child(fun_ast));
@@ -243,7 +243,7 @@ static void add_rmethod_to_subtype(reach_t* r, reach_type_t* t,
   reach_method_name_t* n, reach_method_t* m, pass_opt_t* opt)
 {
   // Add the method to the type if it isn't already there.
-  reach_method_name_t* n2 = add_method_name(t, n->name, false);
+  reach_method_name_t* n2 = add_method_name(t, n->name, opt, false);
   add_rmethod(r, t, n2, m->cap, m->typeargs, opt, false);
 
   // Add this mangling to the type if it isn't already there.
@@ -309,7 +309,7 @@ static void add_rmethod_to_subtypes(reach_t* r, reach_type_t* t,
 
       for(; child != NULL; child = ast_sibling(child))
       {
-        deferred_reification_t* find = lookup_try(NULL, NULL, child, n->name,
+        deferred_reification_t* find = lookup_try(opt, NULL, child, n->name,
           false);
 
         if(find == NULL)
@@ -349,7 +349,7 @@ static reach_method_t* add_rmethod(reach_t* r, reach_type_t* t,
   if(!internal)
   {
     ast_t* r_ast = set_cap_and_ephemeral(t->ast, cap, TK_NONE);
-    deferred_reification_t* fun = lookup(NULL, NULL, r_ast, n->name);
+    deferred_reification_t* fun = lookup(opt, NULL, r_ast, n->name);
     pony_assert(fun != NULL);
 
     // The typeargs and thistype are in the scope of r_ast but we're going to
@@ -414,7 +414,7 @@ static void add_internal(reach_t* r, reach_type_t* t, const char* name,
   pass_opt_t* opt)
 {
   name = stringtab(name);
-  reach_method_name_t* n = add_method_name(t, name, true);
+  reach_method_name_t* n = add_method_name(t, name, opt, true);
   add_rmethod(r, t, n, TK_BOX, NULL, opt, true);
 }
 
@@ -564,7 +564,7 @@ static void add_special(reach_t* r, reach_type_t* t, ast_t* type,
   const char* special, pass_opt_t* opt)
 {
   special = stringtab(special);
-  deferred_reification_t* find = lookup_try(NULL, NULL, type, special, false);
+  deferred_reification_t* find = lookup_try(opt, NULL, type, special, false);
 
   if(find != NULL)
   {
@@ -659,7 +659,7 @@ static void add_fields(reach_t* r, reach_type_t* t, pass_opt_t* opt)
       case TK_FLET:
       case TK_EMBED:
       {
-        deferred_reification_t* member_lookup = lookup(NULL, NULL, t->ast,
+        deferred_reification_t* member_lookup = lookup(opt, NULL, t->ast,
           ast_name(ast_child(member)));
         pony_assert(member_lookup != NULL);
 
@@ -695,7 +695,7 @@ static void add_fields(reach_t* r, reach_type_t* t, pass_opt_t* opt)
 
   if(!has_finaliser && needs_finaliser)
   {
-    reach_method_name_t* n = add_method_name(t, stringtab("_final"), true);
+    reach_method_name_t* n = add_method_name(t, stringtab("_final"), opt, true);
     add_rmethod(r, t, n, TK_BOX, NULL, opt, true);
   }
 }
@@ -912,7 +912,7 @@ static reach_type_t* add_nominal(reach_t* r, ast_t* type, pass_opt_t* opt)
     AST_GET_CHILDREN(bare_method, cap, name, typeparams);
     pony_assert(ast_id(typeparams) == TK_NONE);
 
-    reach_method_name_t* n = add_method_name(t, ast_name(name), false);
+    reach_method_name_t* n = add_method_name(t, ast_name(name), opt, false);
     t->bare_method = add_rmethod(r, t, n, TK_AT, NULL, opt, false);
   }
 
@@ -1323,7 +1323,7 @@ static void reachable_method(reach_t* r, deferred_reification_t* reify,
     t = add_type(r, type, opt);
   }
 
-  reach_method_name_t* n = add_method_name(t, name, false);
+  reach_method_name_t* n = add_method_name(t, name, opt, false);
   reach_method_t* m = add_rmethod(r, t, n, n->cap, typeargs, opt, false);
 
   if((n->id == TK_FUN) && ((n->cap == TK_BOX) || (n->cap == TK_TAG)))

src/libponyc/type/lookup.c

@@ -57,14 +57,14 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from,
   ast_t* orig, ast_t* type, const char* name, bool errors, bool allow_private)
 {
   pony_assert(ast_id(type) == TK_NOMINAL);
+  pony_assert(opt != NULL);
   typecheck_t* t = &opt->check;
 
   ast_t* def = (ast_t*)ast_data(type);
   AST_GET_CHILDREN(def, type_id, typeparams);
   const char* type_name = ast_name(type_id);
 
-  if(is_name_private(type_name) && (from != NULL) && (opt != NULL)
-    && !allow_private)
+  if(is_name_private(type_name) && (from != NULL) && !allow_private)
   {
     if(ast_nearest(def, TK_PACKAGE) != t->frame->package)
     {
@@ -94,34 +94,31 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from,
       case TK_FUN:
       {
         // Typecheck default args immediately.
-        if(opt != NULL)
+        AST_GET_CHILDREN(find, cap, id, typeparams, params);
+        ast_t* param = ast_child(params);
+
+        while(param != NULL)
         {
-          AST_GET_CHILDREN(find, cap, id, typeparams, params);
-          ast_t* param = ast_child(params);
+          AST_GET_CHILDREN(param, name, type, def_arg);
 
-          while(param != NULL)
+          if((ast_id(def_arg) != TK_NONE) && (ast_type(def_arg) == NULL))
           {
-            AST_GET_CHILDREN(param, name, type, def_arg);
-
-            if((ast_id(def_arg) != TK_NONE) && (ast_type(def_arg) == NULL))
-            {
-              ast_t* child = ast_child(def_arg);
+            ast_t* child = ast_child(def_arg);
 
-              if(ast_id(child) == TK_CALL)
-                ast_settype(child, ast_from(child, TK_INFERTYPE));
+            if(ast_id(child) == TK_CALL)
+              ast_settype(child, ast_from(child, TK_INFERTYPE));
 
-              if(ast_visit_scope(&param, pass_pre_expr, pass_expr, opt,
-                PASS_EXPR) != AST_OK)
-                return NULL;
+            if(ast_visit_scope(&param, pass_pre_expr, pass_expr, opt,
+              PASS_EXPR) != AST_OK)
+              return NULL;
 
-              def_arg = ast_childidx(param, 2);
+            def_arg = ast_childidx(param, 2);
 
-              if(!coerce_literals(&def_arg, type, opt))
-                return NULL;
-            }
-
-            param = ast_sibling(param);
+            if(!coerce_literals(&def_arg, type, opt))
+              return NULL;
           }
+
+          param = ast_sibling(param);
         }
         break;
       }
@@ -140,7 +137,7 @@ static deferred_reification_t* lookup_nominal(pass_opt_t* opt, ast_t* from,
     return NULL;
   }
 
-  if(is_name_private(name) && (from != NULL) && (opt != NULL) && !allow_private)
+  if(is_name_private(name) && (from != NULL) && !allow_private)
   {
     switch(ast_id(find))
     {