From 0ba17a5b4608706f696c2212315ea0bb301db209 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 6 Feb 2015 03:41:37 +0100 Subject: [PATCH] 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. --- pygit2/index.py | 8 ++++---- pygit2/remote.py | 20 ++++++++++---------- pygit2/utils.py | 46 +++++++++++++++++++++++----------------------- 3 files changed, 37 insertions(+), 37 deletions(-) diff --git a/pygit2/index.py b/pygit2/index.py index 61b7c9728..47d4cd620 100644 --- a/pygit2/index.py +++ b/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]) diff --git a/pygit2/remote.py b/pygit2/remote.py index c4a195f6a..d2fbdcf9b 100644 --- a/pygit2/remote.py +++ b/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 diff --git a/pygit2/utils.py b/pygit2/utils.py index 3c5fc888b..17582a4d2 100644 --- a/pygit2/utils.py +++ b/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