Safer handling of string arrays

We need to keep hold of the strings which we create. We must also hold
on to the array of strings which we assing to our git_strarray.

We were not doing the latter, which meant that our strings may have been
freed too early, leaving us with with memory access errors (though often
not leading to a crash due to the custom allocator in python).

As we need to keep hold of two/three pieces of information, this looks
like a good place to introduce a context manager. This allows us to
keep these pointers alive without burdening the call sites with a return
of multiple objects they have no use for.

Author: carlosmn

pygit2/index.py

@@ -32,7 +32,7 @@
 from _pygit2 import Oid, Tree, Diff
 from .errors import check_error
 from .ffi import ffi, C
-from .utils import is_string, strings_to_strarray, to_bytes, to_str
+from .utils import is_string, to_bytes, to_str, StrArray
 
 
 class Index(object):
@@ -175,9 +175,9 @@ def add_all(self, pathspecs=[]):
         If pathspecs are specified, only files matching those pathspecs will
         be added.
         """
-        arr, refs = strings_to_strarray(pathspecs)
-        err = C.git_index_add_all(self._index, arr, 0, ffi.NULL, ffi.NULL)
-        check_error(err, True)
+        with StrArray(pathspecs) as arr:
+            err = C.git_index_add_all(self._index, arr, 0, ffi.NULL, ffi.NULL)
+            check_error(err, True)
 
     def add(self, path_or_entry):
         """add([path|entry])

pygit2/remote.py

@@ -34,7 +34,7 @@
 from .ffi import ffi, C
 from .credentials import KeypairFromAgent
 from .refspec import Refspec
-from .utils import to_bytes, strarray_to_strings, strings_to_strarray
+from .utils import to_bytes, strarray_to_strings, StrArray
 
 
 def maybe_string(ptr):
@@ -253,9 +253,9 @@ def fetch_refspecs(self):
 
     @fetch_refspecs.setter
     def fetch_refspecs(self, l):
-        arr, refs = strings_to_strarray(l)
-        err = C.git_remote_set_fetch_refspecs(self._remote, arr)
-        check_error(err)
+        with StrArray(l) as arr:
+            err = C.git_remote_set_fetch_refspecs(self._remote, arr)
+            check_error(err)
 
     @property
     def push_refspecs(self):
@@ -269,9 +269,9 @@ def push_refspecs(self):
 
     @push_refspecs.setter
     def push_refspecs(self, l):
-        arr, refs = strings_to_strarray(l)
-        err = C.git_remote_set_push_refspecs(self._remote, arr)
-        check_error(err)
+        with StrArray(l) as arr:
+            err = C.git_remote_set_push_refspecs(self._remote, arr)
+            check_error(err)
 
     def add_fetch(self, spec):
         """add_fetch(refspec)
@@ -305,7 +305,6 @@ def push(self, specs, signature=None, message=None):
         err = C.git_remote_init_callbacks(defaultcallbacks, 1)
         check_error(err)
 
-        refspecs, refspecs_refs = strings_to_strarray(specs)
         if signature:
             sig_cptr = ffi.new('git_signature **')
             ffi.buffer(sig_cptr)[:] = signature._pointer[:]
@@ -333,8 +332,9 @@ def push(self, specs, signature=None, message=None):
             raise
 
         try:
-            err = C.git_remote_push(self._remote, refspecs, ffi.NULL, sig_ptr, to_bytes(message))
-            check_error(err)
+            with StrArray(specs) as refspecs:
+                err = C.git_remote_push(self._remote, refspecs, ffi.NULL, sig_ptr, to_bytes(message))
+                check_error(err)
         finally:
             self._self_handle = None
 

pygit2/utils.py

@@ -50,34 +50,34 @@ def strarray_to_strings(arr):
     return l
 
 
-def strings_to_strarray(l):
-    """Convert a list of strings to a git_strarray
+class StrArray(object):
+    """A git_strarray wrapper
 
-    We return first the git_strarray* you can pass to libgit2 and a
-    list of references to the memory, which we must keep around for as
-    long as the git_strarray must live.
-    """
+    Use this in order to get a git_strarray* to pass to libgit2 out of a
+    list of strings. This has a context manager, which you should use, e.g.
 
-    if not isinstance(l, list):
-        raise TypeError("Value must be a list")
+        with StrArray(list_of_strings) as arr:
+            C.git_function_that_takes_strarray(arr)
+    """
 
-    arr = ffi.new('git_strarray *')
-    strings = ffi.new('char *[]', len(l))
+    def __init__(self, l):
+        if not isinstance(l, list):
+            raise TypeError("Value must be a list")
 
-    # We need refs in order to keep a reference to the value returned
-    # by the ffi.new(). Otherwise, they will be freed and the memory
-    # re-used, with less than great consequences.
-    refs = [None] * len(l)
+        arr = ffi.new('git_strarray *')
+        strings = [None] * len(l)
+        for i in range(len(l)):
+            if not is_string(l[i]):
+                raise TypeError("Value must be a string")
 
-    for i in range(len(l)):
-        if not is_string(l[i]):
-            raise TypeError("Value must be a string")
+            strings[i] = ffi.new('char []', to_bytes(l[i]))
 
-        s = ffi.new('char []', to_bytes(l[i]))
-        refs[i] = s
-        strings[i] = s
+        self._arr = ffi.new('char *[]', strings)
+        self._strings = strings
+        self.array = ffi.new('git_strarray *', [self._arr, len(strings)])
 
-    arr.strings = strings
-    arr.count = len(l)
+    def __enter__(self):
+        return self.array
 
-    return arr, refs
+    def __exit__(self, type, value, traceback):
+        pass