From 5a20510f8a73ec1603aac33652b64016b0502748 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Mon, 14 Apr 2014 02:13:36 +0200 Subject: [PATCH] Safer repository pointer extraction Casting a pointer to a non-pointer type is something which you should never do. Instead, do something a bit more convoluted, but which is guaranteed to give us the right pointer, as well as making sure that the memory we exchange between python/cffi and the C extension is of the right pointer size. While we're touching this code, fix which object we pass to the Remote constructor to keep alive. We need to pass the Repository object to stop it from becoming unreferenced (and thus freeing memory the remote needs) instead of the git_repository pointer. --- pygit2/repository.py | 25 ++++++++++++++----------- src/repository.c | 9 ++------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/pygit2/repository.py b/pygit2/repository.py index a22c820..107db91 100644 --- a/pygit2/repository.py +++ b/pygit2/repository.py @@ -41,6 +41,15 @@ from .remote import Remote class Repository(_Repository): + def __init__(self, *args, **kwargs): + super(Repository, self).__init__(*args, **kwargs) + + # Get the pointer as the contents of a buffer and store it for + # later access + repo_cptr = ffi.new('git_repository **') + ffi.buffer(repo_cptr)[:] = self._pointer[:] + self._repo = repo_cptr[0] + # # Mapping interface # @@ -72,36 +81,30 @@ class Repository(_Repository): Creates a new remote. """ - repo_cptr = ffi.new('git_repository **') - repo_cptr[0] = ffi.cast('git_repository *', self._pointer) cremote = ffi.new('git_remote **') - repo = repo_cptr[0] - err = C.git_remote_create(cremote, repo, to_str(name), to_str(url)) + err = C.git_remote_create(cremote, self._repo, to_str(name), to_str(url)) check_error(err) - return Remote(repo, cremote[0]) + return Remote(self, cremote[0]) @property def remotes(self): """Returns all configured remotes""" - repo_cptr = ffi.new('git_repository **') - repo_cptr[0] = ffi.cast('git_repository *', self._pointer) names = ffi.new('git_strarray *') - repo = repo_cptr[0] try: - err = C.git_remote_list(names, repo) + err = C.git_remote_list(names, self._repo) check_error(err) l = [None] * names.count cremote = ffi.new('git_remote **') for i in range(names.count): - err = C.git_remote_load(cremote, repo, names.strings[i]) + err = C.git_remote_load(cremote, self._repo, names.strings[i]) check_error(err) - l[i] = Remote(repo, cremote[0]) + l[i] = Remote(self, cremote[0]) return l finally: C.git_strarray_free(names) diff --git a/src/repository.c b/src/repository.c index e8e38f9..1713136 100644 --- a/src/repository.c +++ b/src/repository.c @@ -1293,13 +1293,8 @@ PyDoc_STRVAR(Repository__pointer__doc__, "Get the repo's pointer. For internal u PyObject * Repository__pointer__get__(Repository *self) { - /* - * This is pretty bad. We shouldn't be casting a pointer into an - * integer, but we can't access the contents of a PyCapsule from - * python code, which we need to do in order to get a type that - * cffi likes. - */ - return PyLong_FromLongLong((long long) self->repo); + /* Bytes means a raw buffer */ + return PyBytes_FromStringAndSize((char *) &self->repo, sizeof(git_repository *)); } PyDoc_STRVAR(Repository_checkout_head__doc__,