diff --git a/Makefile b/Makefile index af6e6cfcd..b6a6dee4d 100644 --- a/Makefile +++ b/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 diff --git a/docs/development.rst b/docs/development.rst index e8a0f66d2..8fe575cc2 100644 --- a/docs/development.rst +++ b/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 diff --git a/misc/valgrind-python.supp b/misc/valgrind-python.supp index 15982cc3e..bc8f77f26 100644 --- a/misc/valgrind-python.supp +++ b/misc/valgrind-python.supp @@ -5,13 +5,13 @@ # # 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. @@ -19,25 +19,25 @@ { 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 +} diff --git a/src/error.c b/src/error.c index 01503b6b0..c6e5e6c75 100644 --- a/src/error.c +++ b/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) diff --git a/src/options.c b/src/options.c index 4f3a5deab..1399975aa 100644 --- a/src/options.c +++ b/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); diff --git a/src/pygit2.c b/src/pygit2.c index 316d88254..ab5e013ac 100644 --- a/src/pygit2.c +++ b/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; } diff --git a/src/utils.c b/src/utils.c index 7e3128989..1277c2f99 100644 --- a/src/utils.c +++ b/src/utils.c @@ -37,23 +37,25 @@ extern PyTypeObject BlobType; extern PyTypeObject TagType; /** - * py_str_to_c_str() returns a newly allocated C string holding the string + * py_str_to_c_str() returns a *newly allocated* C string holding the string * contained in the 'value' argument. */ char * py_str_to_c_str(PyObject *value, const char *encoding) { - const char *borrowed; - char *c_str = NULL; PyObject *tmp = NULL; - borrowed = py_str_borrow_c_str(&tmp, value, encoding); + const char *borrowed = py_str_borrow_c_str(&tmp, value, encoding); if (!borrowed) return NULL; - c_str = strdup(borrowed); - + char *c_str = strdup(borrowed); Py_DECREF(tmp); + + if (!c_str) { + // FIXME Set exception + } + return c_str; } @@ -64,27 +66,32 @@ py_str_to_c_str(PyObject *value, const char *encoding) const char * py_str_borrow_c_str(PyObject **tvalue, PyObject *value, const char *encoding) { - /* Case 1: byte string */ - if (PyBytes_Check(value)) { - Py_INCREF(value); - *tvalue = value; - return PyBytes_AsString(value); - } + PyObject *py_str; - /* Case 2: text string */ - if (PyUnicode_Check(value)) { - if (encoding == NULL) - *tvalue = PyUnicode_AsUTF8String(value); - else - *tvalue = PyUnicode_AsEncodedString(value, encoding, "strict"); - if (*tvalue == NULL) + // Get new PyBytes reference from value + if (PyUnicode_Check(value)) { // Text string + py_str = (encoding) ? PyUnicode_AsEncodedString(value, encoding, "strict") + : PyUnicode_AsUTF8String(value); + if (py_str == NULL) return NULL; - return PyBytes_AsString(*tvalue); + } else if (PyBytes_Check(value)) { // Byte string + py_str = value; + Py_INCREF(py_str); + } else { // Type error + Error_type_error("unexpected %.200s", value); + return NULL; } - /* Type error */ - Error_type_error("unexpected %.200s", value); - return NULL; + // Borrow c string from the new PyBytes reference + char *c_str = PyBytes_AsString(py_str); + if (c_str == NULL) { + Py_DECREF(py_str); + return NULL; + } + + // Return the borrowed c string and the new PyBytes reference + *tvalue = py_str; + return c_str; } /** diff --git a/src/utils.h b/src/utils.h index 1a066889f..6b068d2ab 100644 --- a/src/utils.h +++ b/src/utils.h @@ -152,6 +152,15 @@ int py_object_to_otype(PyObject *py_type); if (PyModule_AddObject(module, #type, (PyObject*) & type ## Type) == -1)\ return NULL; +#define ADD_EXC(m, name, base)\ + name = PyErr_NewException("_pygit2." #name, base, NULL);\ + if (name == NULL) goto fail;\ + Py_INCREF(name);\ + if (PyModule_AddObject(m, #name, name)) {\ + Py_DECREF(name);\ + goto fail;\ + } + #define ADD_CONSTANT_INT(m, name) \ if (PyModule_AddIntConstant(m, #name, name) == -1) return NULL;