[RFA] Fix leak detected in python.c initialization code.

Message ID 20190908074922.29305-1-philippe.waroquiers@skynet.be
State New, archived
Headers

Commit Message

Philippe Waroquiers Sept. 8, 2019, 7:49 a.m. UTC
  Valgrind reports the below leak.
Make the variable progname_copy static, so that Valgrind continues
to find a pointer to the memory given to Python.
Note that the comment in do_start_initialization and the Python documentation
indicates that the progname given to Py_SetProgramName cannot be freed.
However, in Python 3.7.4, Py_SetProgramName does:
void
Py_SetProgramName(const wchar_t *program_name)
{
    ...
    PyMem_RawFree(_Py_path_config.program_name);
    _Py_path_config.program_name = _PyMem_RawWcsdup(program_name);

So, it looks like 3.7.4 Python duplicates its argument, which explains
the leak found by Valgrind.
It looks better to respect the doc and not have GDB freeing the string
given to Py_SetProgramName, and avoid the leak error by declaring
the progname_copy static.
This will work with Python versions that really use this string without
duplicating it, and avoids a leak report for Python version that duplicates
it.

==4023== 200 bytes in 1 blocks are definitely lost in loss record 4,545 of 7,116^M
==4023==    at 0x4C29F33: malloc (vg_replace_malloc.c:307)^M
==4023==    by 0x446D27: xmalloc (alloc.c:60)^M
==4023==    by 0x657C77: do_start_initialization (python.c:1610)^M
==4023==    by 0x657C77: _initialize_python() (python.c:1823)^M
==4023==    by 0x75FE24: initialize_all_files() (init.c:231)^M
==4023==    by 0x708A94: gdb_init(char*) (top.c:2242)^M
==4023==    by 0x5E7460: captured_main_1 (main.c:857)^M
==4023==    by 0x5E7460: captured_main (main.c:1161)^M
==4023==    by 0x5E7460: gdb_main(captured_main_args*) (main.c:1186)^M
==4023==    by 0x4122D4: main (gdb.c:32)^M

gdb/ChangeLog
YYYY-MM-DD  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* python/python.c (do_start_initialization): Make progname_copy static,
	to avoid a leak report.
---
 gdb/python/python.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Tom Tromey Sept. 9, 2019, 4:39 p.m. UTC | #1
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> So, it looks like 3.7.4 Python duplicates its argument, which explains
Philippe> the leak found by Valgrind.
Philippe> It looks better to respect the doc and not have GDB freeing the string
Philippe> given to Py_SetProgramName, and avoid the leak error by declaring
Philippe> the progname_copy static.
Philippe> This will work with Python versions that really use this string without
Philippe> duplicating it, and avoids a leak report for Python version that duplicates
Philippe> it.

I think something along these lines should be put into a comment just
before the declaration.  This is ok with that change.

thanks,
Tom
  
Philippe Waroquiers Sept. 9, 2019, 9:53 p.m. UTC | #2
On Mon, 2019-09-09 at 10:39 -0600, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> 
> Philippe> So, it looks like 3.7.4 Python duplicates its argument, which explains
> Philippe> the leak found by Valgrind.
> Philippe> It looks better to respect the doc and not have GDB freeing the string
> Philippe> given to Py_SetProgramName, and avoid the leak error by declaring
> Philippe> the progname_copy static.
> Philippe> This will work with Python versions that really use this string without
> Philippe> duplicating it, and avoids a leak report for Python version that duplicates
> Philippe> it.
> 
> I think something along these lines should be put into a comment just
> before the declaration.  This is ok with that change.
> 
> thanks,
> Tom
Here is the comment I added:
-  wchar_t *progname_copy;
+  /* Python documentation indicates that the memory given
+     to Py_SetProgramName cannot be freed.  However, it seems that
+     at least Python 3.7.4 Py_SetProgramName takes a copy of the
+     given program_name.  Making progname_copy static and not release
+     the memory avoids a leak report for Python versions that duplicate
+     program_name, and respect the requirement of Py_SetProgramName
+     for Python versions that do not duplicate program_name.  */
+  static wchar_t *progname_copy;

Pushed the above

Thanks for the review

Philippe
  

Patch

diff --git a/gdb/python/python.c b/gdb/python/python.c
index b309ae91ba..1d9178ebcc 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1590,7 +1590,7 @@  do_start_initialization ()
 {
 #ifdef IS_PY3K
   size_t progsize, count;
-  wchar_t *progname_copy;
+  static wchar_t *progname_copy;
 #endif
 
 #ifdef WITH_PYTHON_PATH