Start review for memory issues, with valgrind

Update valgrind suppression file for Python 3.7
Fix a memory leak in option(..)

TODO: Review all calls to py_str_to_c_str, the returned value must be
freed after use.

Makefile

@@ -1,7 +1,7 @@
 .PHONY: build
 
 build:
-	python setup.py build_ext --inplace
+	python setup.py build_ext --inplace -g
 
 html:
 	cd docs && find . -name "*rst" | entr make html

docs/development.rst

@@ -45,4 +45,25 @@ Example::
 Running Valgrind
 ===================================
 
-valgrind --tool=memcheck --suppressions=misc/valgrind-python.supp ~/Python-3.7.4/bin/pytest
+Step 1. Build libc and libgit2 with debug symbols. See your distribution
+documentation.
+
+Step 2. Build Python to be used with valgrind, e.g.::
+
+  $ ./configure --prefix=~/Python-3.7.4 --without-pymalloc --with-pydebug --with-valgrind
+  $ make
+  $ make install
+  $ export PYTHONBIN=~/Python-3.7.4/bin
+
+Step 3. Build pygit2 with debug symbols::
+
+  $ rm build -rf && $PYTHONBIN/python3 setup.py build_ext --inplace -g
+
+Step 4. Install requirements::
+
+  $ $PYTHONBIN/python3 setup.py install
+  $ pip insall pytest
+
+Step 4. Run valgrind:
+
+  $ valgrind -v --leak-check=full --suppressions=misc/valgrind-python.supp $PYTHONBIN/pytest &> valgrind.txt

misc/valgrind-python.supp

@@ -5,39 +5,39 @@
 #
 #	cd python/dist/src
 #	valgrind --tool=memcheck --suppressions=Misc/valgrind-python.supp \
-#		./python -E -tt ./Lib/test/regrtest.py -u bsddb,network
+#		./python -E ./Lib/test/regrtest.py -u gui,network
 #
 # You must edit Objects/obmalloc.c and uncomment Py_USING_MEMORY_DEBUGGER
-# to use the preferred suppressions with Py_ADDRESS_IN_RANGE.
+# to use the preferred suppressions with address_in_range.
 #
 # If you do not want to recompile Python, you can uncomment
-# suppressions for PyObject_Free and PyObject_Realloc.
+# suppressions for _PyObject_Free and _PyObject_Realloc.
 #
 # See Misc/README.valgrind for more information.
 
 # all tool names: Addrcheck,Memcheck,cachegrind,helgrind,massif
 {
    ADDRESS_IN_RANGE/Invalid read of size 4
    Memcheck:Addr4
-   fun:Py_ADDRESS_IN_RANGE
+   fun:address_in_range
 }
 
 {
    ADDRESS_IN_RANGE/Invalid read of size 4
    Memcheck:Value4
-   fun:Py_ADDRESS_IN_RANGE
+   fun:address_in_range
 }
 
 {
    ADDRESS_IN_RANGE/Invalid read of size 8 (x86_64 aka amd64)
    Memcheck:Value8
-   fun:Py_ADDRESS_IN_RANGE
+   fun:address_in_range
 }
 
 {
    ADDRESS_IN_RANGE/Conditional jump or move depends on uninitialised value
    Memcheck:Cond
-   fun:Py_ADDRESS_IN_RANGE
+   fun:address_in_range
 }
 
 #
@@ -124,41 +124,65 @@
    fun:_dl_allocate_tls
 }
 
-{
-   ADDRESS_IN_RANGE/Invalid read of size 4
-   Memcheck:Addr4
-   fun:PyObject_Free
-}
-
-{
-   ADDRESS_IN_RANGE/Invalid read of size 4
-   Memcheck:Value4
-   fun:PyObject_Free
-}
-
-{
-   ADDRESS_IN_RANGE/Conditional jump or move depends on uninitialised value
-   Memcheck:Cond
-   fun:PyObject_Free
-}
-
-{
-   ADDRESS_IN_RANGE/Invalid read of size 4
-   Memcheck:Addr4
-   fun:PyObject_Realloc
-}
-
-{
-   ADDRESS_IN_RANGE/Invalid read of size 4
-   Memcheck:Value4
-   fun:PyObject_Realloc
-}
+###{
+###   ADDRESS_IN_RANGE/Invalid read of size 4
+###   Memcheck:Addr4
+###   fun:_PyObject_Free
+###}
+###
+###{
+###   ADDRESS_IN_RANGE/Invalid read of size 4
+###   Memcheck:Value4
+###   fun:_PyObject_Free
+###}
+###
+###{
+###   ADDRESS_IN_RANGE/Use of uninitialised value of size 8
+###   Memcheck:Addr8
+###   fun:_PyObject_Free
+###}
+###
+###{
+###   ADDRESS_IN_RANGE/Use of uninitialised value of size 8
+###   Memcheck:Value8
+###   fun:_PyObject_Free
+###}
+###
+###{
+###   ADDRESS_IN_RANGE/Conditional jump or move depends on uninitialised value
+###   Memcheck:Cond
+###   fun:_PyObject_Free
+###}
 
-{
-   ADDRESS_IN_RANGE/Conditional jump or move depends on uninitialised value
-   Memcheck:Cond
-   fun:PyObject_Realloc
-}
+###{
+###   ADDRESS_IN_RANGE/Invalid read of size 4
+###   Memcheck:Addr4
+###   fun:_PyObject_Realloc
+###}
+###
+###{
+###   ADDRESS_IN_RANGE/Invalid read of size 4
+###   Memcheck:Value4
+###   fun:_PyObject_Realloc
+###}
+###
+###{
+###   ADDRESS_IN_RANGE/Use of uninitialised value of size 8
+###   Memcheck:Addr8
+###   fun:_PyObject_Realloc
+###}
+###
+###{
+###   ADDRESS_IN_RANGE/Use of uninitialised value of size 8
+###   Memcheck:Value8
+###   fun:_PyObject_Realloc
+###}
+###
+###{
+###   ADDRESS_IN_RANGE/Conditional jump or move depends on uninitialised value
+###   Memcheck:Cond
+###   fun:_PyObject_Realloc
+###}
 
 ###
 ### All the suppressions below are for errors that occur within libraries
@@ -286,6 +310,38 @@
 ###   fun:MD5_Update
 ###}
 
+# Fedora's package "openssl-1.0.1-0.1.beta2.fc17.x86_64" on x86_64
+# See http://bugs.python.org/issue14171
+{
+   openssl 1.0.1 prng 1
+   Memcheck:Cond
+   fun:bcmp
+   fun:fips_get_entropy
+   fun:FIPS_drbg_instantiate
+   fun:RAND_init_fips
+   fun:OPENSSL_init_library
+   fun:SSL_library_init
+   fun:init_hashlib
+}
+
+{
+   openssl 1.0.1 prng 2
+   Memcheck:Cond
+   fun:fips_get_entropy
+   fun:FIPS_drbg_instantiate
+   fun:RAND_init_fips
+   fun:OPENSSL_init_library
+   fun:SSL_library_init
+   fun:init_hashlib
+}
+
+{
+   openssl 1.0.1 prng 3
+   Memcheck:Value8
+   fun:_x86_64_AES_encrypt_compact
+   fun:AES_encrypt
+}
+
 #
 # All of these problems come from using test_socket_ssl
 #
@@ -388,4 +444,37 @@
    fun:SHA1_Update
 }
 
+{
+   test_buffer_non_debug
+   Memcheck:Addr4
+   fun:PyUnicodeUCS2_FSConverter
+}
+
+{
+   test_buffer_non_debug
+   Memcheck:Addr4
+   fun:PyUnicode_FSConverter
+}
+
+{
+   wcscmp_false_positive
+   Memcheck:Addr8
+   fun:wcscmp
+   fun:_PyOS_GetOpt
+   fun:Py_Main
+   fun:main
+}
+
+# Additional suppressions for the unified decimal tests:
+{
+   test_decimal
+   Memcheck:Addr4
+   fun:PyUnicodeUCS2_FSConverter
+}
+
+{
+   test_decimal2
+   Memcheck:Addr4
+   fun:PyUnicode_FSConverter
+}
 

src/error.c

@@ -27,9 +27,9 @@
 
 #include "error.h"
 
-PyObject *GitError;
-PyObject *AlreadyExistsError;
-PyObject *InvalidSpecError;
+extern PyObject *GitError;
+extern PyObject *AlreadyExistsError;
+extern PyObject *InvalidSpecError;
 
 PyObject *
 Error_type(int type)

src/options.c

@@ -264,11 +264,15 @@ option(PyObject *self, PyObject *args)
         case GIT_OPT_SET_SSL_CERT_LOCATIONS:
         {
             PyObject *py_file, *py_dir;
-            const char *file_path, *dir_path;
+            char *file_path, *dir_path;
             int err;
 
             py_file = PyTuple_GetItem(args, 1);
+            if (!py_file)
+                return NULL;
             py_dir = PyTuple_GetItem(args, 2);
+            if (!py_dir)
+                return NULL;
 
             /* py_file and py_dir are only valid if they are strings */
             if (PyUnicode_Check(py_file) || PyBytes_Check(py_file)) {
@@ -284,6 +288,8 @@ option(PyObject *self, PyObject *args)
             }
 
             err = git_libgit2_opts(GIT_OPT_SET_SSL_CERT_LOCATIONS, file_path, dir_path);
+            free(file_path);
+            free(dir_path);
 
             if (err < 0)
                 return Error_set(err);

src/pygit2.c

@@ -36,9 +36,9 @@
 #include "oid.h"
 #include "options.h"
 
-extern PyObject *GitError;
-extern PyObject *AlreadyExistsError;
-extern PyObject *InvalidSpecError;
+PyObject *GitError;
+PyObject *AlreadyExistsError;
+PyObject *InvalidSpecError;
 
 extern PyTypeObject RepositoryType;
 extern PyTypeObject OdbType;
@@ -250,9 +250,23 @@ PyMethodDef module_methods[] = {
     {NULL}
 };
 
-PyObject *
-moduleinit(PyObject* m)
+
+struct PyModuleDef moduledef = {
+    PyModuleDef_HEAD_INIT,
+    "_pygit2",                       /* m_name */
+    "Python bindings for libgit2.",  /* m_doc */
+    -1,                              /* m_size */
+    module_methods,                  /* m_methods */
+    NULL,                            /* m_reload */
+    NULL,                            /* m_traverse */
+    NULL,                            /* m_clear */
+    NULL,                            /* m_free */
+};
+
+PyMODINIT_FUNC
+PyInit__pygit2(void)
 {
+    PyObject *m = PyModule_Create(&moduledef);
     if (m == NULL)
         return NULL;
 
@@ -292,18 +306,10 @@ moduleinit(PyObject* m)
     ADD_CONSTANT_INT(m, GIT_OPT_SET_PACK_MAX_OBJECTS);
     ADD_CONSTANT_INT(m, GIT_OPT_DISABLE_PACK_KEEP_FILE_CHECKS);
 
-    /* Errors */
-    GitError = PyErr_NewException("_pygit2.GitError", NULL, NULL);
-    Py_INCREF(GitError);
-    PyModule_AddObject(m, "GitError", GitError);
-
-    AlreadyExistsError = PyErr_NewException("_pygit2.AlreadyExistsError", PyExc_ValueError, NULL);
-    Py_INCREF(AlreadyExistsError);
-    PyModule_AddObject(m, "AlreadyExistsError", AlreadyExistsError);
-
-    InvalidSpecError = PyErr_NewException("_pygit2.InvalidSpecError", PyExc_ValueError, NULL);
-    Py_INCREF(InvalidSpecError);
-    PyModule_AddObject(m, "InvalidSpecError", InvalidSpecError);
+    /* Exceptions */
+    ADD_EXC(m, GitError, NULL);
+    ADD_EXC(m, AlreadyExistsError, PyExc_ValueError);
+    ADD_EXC(m, InvalidSpecError, PyExc_ValueError);
 
     /* Repository */
     INIT_TYPE(RepositoryType, NULL, PyType_GenericNew)
@@ -574,25 +580,8 @@ moduleinit(PyObject* m)
     git_libgit2_init();
 
     return m;
-}
-
-
-struct PyModuleDef moduledef = {
-    PyModuleDef_HEAD_INIT,
-    "_pygit2",                       /* m_name */
-    "Python bindings for libgit2.",  /* m_doc */
-    -1,                              /* m_size */
-    module_methods,                  /* m_methods */
-    NULL,                            /* m_reload */
-    NULL,                            /* m_traverse */
-    NULL,                            /* m_clear */
-    NULL,                            /* m_free */
-};
 
-PyMODINIT_FUNC
-PyInit__pygit2(void)
-{
-    PyObject* m;
-    m = PyModule_Create(&moduledef);
-    return moduleinit(m);
+fail:
+    Py_DECREF(m);
+    return NULL;
 }