* [PATCH] python: Fix memleak in do_start_initialization @ 2017-11-26 3:53 Simon Marchi 2017-11-26 19:00 ` Tom Tromey 0 siblings, 1 reply; 5+ messages in thread From: Simon Marchi @ 2017-11-26 3:53 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi While playing with valgrind, I noticed that the progname variable in do_start_initialization is not being freed (concat returns a malloc'ed string). Use a unique_xmalloc_ptr to manage it. gdb/ChangeLog: * python/python.c (do_start_initialization): Change progname type to gdb::unique_xmalloc_ptr. --- gdb/python/python.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/gdb/python/python.c b/gdb/python/python.c index 5f152611e8..f17ac36569 100644 --- a/gdb/python/python.c +++ b/gdb/python/python.c @@ -1658,7 +1658,7 @@ finalize_python (void *ignore) static bool do_start_initialization () { - char *progname; + gdb::unique_xmalloc_ptr<char> progname; #ifdef IS_PY3K int i; size_t progsize, count; @@ -1672,19 +1672,19 @@ do_start_initialization () /foo/bin/python /foo/lib/pythonX.Y/... This must be done before calling Py_Initialize. */ - progname = concat (ldirname (python_libdir).c_str (), SLASH_STRING, "bin", - SLASH_STRING, "python", (char *) NULL); + progname.reset (concat (ldirname (python_libdir).c_str (), SLASH_STRING, + "bin", SLASH_STRING, "python", (char *) NULL)); #ifdef IS_PY3K std::string oldloc = setlocale (LC_ALL, NULL); setlocale (LC_ALL, ""); - progsize = strlen (progname); + progsize = strlen (progname.get ()); progname_copy = (wchar_t *) PyMem_Malloc ((progsize + 1) * sizeof (wchar_t)); if (!progname_copy) { fprintf (stderr, "out of memory\n"); return false; } - count = mbstowcs (progname_copy, progname, progsize + 1); + count = mbstowcs (progname_copy, progname.get (), progsize + 1); if (count == (size_t) -1) { fprintf (stderr, "Could not convert python path to string\n"); -- 2.15.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] python: Fix memleak in do_start_initialization 2017-11-26 3:53 [PATCH] python: Fix memleak in do_start_initialization Simon Marchi @ 2017-11-26 19:00 ` Tom Tromey 2017-11-26 20:56 ` Simon Marchi 0 siblings, 1 reply; 5+ messages in thread From: Tom Tromey @ 2017-11-26 19:00 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches >>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes: Simon> While playing with valgrind, I noticed that the progname variable in Simon> do_start_initialization is not being freed (concat returns a malloc'ed Simon> string). Use a unique_xmalloc_ptr to manage it. I don't think this is quite correct. On the Python 2 branch, the code does this: Py_SetProgramName (progname); The docs for Py_SetProgramName say: The argument should point to a zero-terminated character string in static storage whose contents will not change for the duration of the program's execution. I think a comment referring to the Python docs would be better. And, freeing progname on the Python 3 branch is ok. Maybe the Python 2 branch could pass "progname.release ()". Tom ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] python: Fix memleak in do_start_initialization 2017-11-26 19:00 ` Tom Tromey @ 2017-11-26 20:56 ` Simon Marchi 2017-11-26 21:55 ` Tom Tromey 0 siblings, 1 reply; 5+ messages in thread From: Simon Marchi @ 2017-11-26 20:56 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 2017-11-26 02:00 PM, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes: > > Simon> While playing with valgrind, I noticed that the progname variable in > Simon> do_start_initialization is not being freed (concat returns a malloc'ed > Simon> string). Use a unique_xmalloc_ptr to manage it. > > I don't think this is quite correct. > On the Python 2 branch, the code does this: > > Py_SetProgramName (progname); > > The docs for Py_SetProgramName say: > > The argument should point to a > zero-terminated character string in static storage whose contents > will not change for the duration of the program's execution. Ahh, you are completely right, I always forget about Python 2. And I relied too much on my IDE to find usages of progname, it didn't find the usage in the disabled #if branch. At least it would have failed at compile time with Python 2, not runtime. > I think a comment referring to the Python docs would be better. > And, freeing progname on the Python 3 branch is ok. > Maybe the Python 2 branch could pass "progname.release ()". There's already a comment in that regard: /* Note that Py_SetProgramName expects the string it is passed to remain alive for the duration of the program's execution, so it is not freed after this call. */ When I read it, I thought it referred only to progname_copy, but it also applies to progname for Python 2. I integrated your suggestion, wdyt? From 468fe44d21dfed10f547d790d233547ab0da2cf5 Mon Sep 17 00:00:00 2001 From: Simon Marchi <simon.marchi@polymtl.ca> Date: Sat, 25 Nov 2017 22:51:02 -0500 Subject: [PATCH] python: Fix memleak in do_start_initialization While playing with valgrind, I noticed that with Python 3, the progname variable in do_start_initialization is not being freed (concat returns a malloc'ed string). This patch uses unique_xmalloc_ptr to manage it. With Python 2, we pass progname it directly to Py_SetProgramName, so it should not be freed. We therefore release it before passing it. gdb/ChangeLog: * python/python.c (do_start_initialization): Change progname type to gdb::unique_xmalloc_ptr. Release the pointer when using Python 2. --- gdb/python/python.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/gdb/python/python.c b/gdb/python/python.c index 5f152611e8..6b05f97b39 100644 --- a/gdb/python/python.c +++ b/gdb/python/python.c @@ -1658,7 +1658,7 @@ finalize_python (void *ignore) static bool do_start_initialization () { - char *progname; + gdb::unique_xmalloc_ptr<char> progname; #ifdef IS_PY3K int i; size_t progsize, count; @@ -1672,19 +1672,19 @@ do_start_initialization () /foo/bin/python /foo/lib/pythonX.Y/... This must be done before calling Py_Initialize. */ - progname = concat (ldirname (python_libdir).c_str (), SLASH_STRING, "bin", - SLASH_STRING, "python", (char *) NULL); + progname.reset (concat (ldirname (python_libdir).c_str (), SLASH_STRING, + "bin", SLASH_STRING, "python", (char *) NULL)); #ifdef IS_PY3K std::string oldloc = setlocale (LC_ALL, NULL); setlocale (LC_ALL, ""); - progsize = strlen (progname); + progsize = strlen (progname.get ()); progname_copy = (wchar_t *) PyMem_Malloc ((progsize + 1) * sizeof (wchar_t)); if (!progname_copy) { fprintf (stderr, "out of memory\n"); return false; } - count = mbstowcs (progname_copy, progname, progsize + 1); + count = mbstowcs (progname_copy, progname.get (), progsize + 1); if (count == (size_t) -1) { fprintf (stderr, "Could not convert python path to string\n"); @@ -1697,7 +1697,7 @@ do_start_initialization () it is not freed after this call. */ Py_SetProgramName (progname_copy); #else - Py_SetProgramName (progname); + Py_SetProgramName (progname.release ()); #endif #endif -- 2.15.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] python: Fix memleak in do_start_initialization 2017-11-26 20:56 ` Simon Marchi @ 2017-11-26 21:55 ` Tom Tromey 2017-11-27 0:36 ` Simon Marchi 0 siblings, 1 reply; 5+ messages in thread From: Tom Tromey @ 2017-11-26 21:55 UTC (permalink / raw) To: Simon Marchi; +Cc: Tom Tromey, gdb-patches >>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes: Simon> There's already a comment in that regard: Hah, I totally missed this. Simon> When I read it, I thought it referred only to progname_copy, but it Simon> also applies to progname for Python 2. I integrated your suggestion, Simon> wdyt? Simon> - char *progname; Simon> + gdb::unique_xmalloc_ptr<char> progname; I think the patch looks good, but would be even clearer if this declaration were moved into the #if, so that.. Simon> - progname = concat (ldirname (python_libdir).c_str (), SLASH_STRING, "bin", Simon> - SLASH_STRING, "python", (char *) NULL); Simon> + progname.reset (concat (ldirname (python_libdir).c_str (), SLASH_STRING, Simon> + "bin", SLASH_STRING, "python", (char *) NULL)); ...this reset could be replaced with plain initialization. thanks, Tom ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] python: Fix memleak in do_start_initialization 2017-11-26 21:55 ` Tom Tromey @ 2017-11-27 0:36 ` Simon Marchi 0 siblings, 0 replies; 5+ messages in thread From: Simon Marchi @ 2017-11-27 0:36 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 2017-11-26 04:54 PM, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes: > > Simon> There's already a comment in that regard: > > Hah, I totally missed this. > > Simon> When I read it, I thought it referred only to progname_copy, but it > Simon> also applies to progname for Python 2. I integrated your suggestion, > Simon> wdyt? > > Simon> - char *progname; > Simon> + gdb::unique_xmalloc_ptr<char> progname; > > I think the patch looks good, but would be even clearer if this > declaration were moved into the #if, so that.. > > Simon> - progname = concat (ldirname (python_libdir).c_str (), SLASH_STRING, "bin", > Simon> - SLASH_STRING, "python", (char *) NULL); > Simon> + progname.reset (concat (ldirname (python_libdir).c_str (), SLASH_STRING, > Simon> + "bin", SLASH_STRING, "python", (char *) NULL)); > > ...this reset could be replaced with plain initialization. > > thanks, > Tom > Good idea, here's what I pushed. Thanks for the comments! From e8e7d10c39955e7ff99ff42f6f16d6befe2fa12e Mon Sep 17 00:00:00 2001 From: Simon Marchi <simon.marchi@polymtl.ca> Date: Sun, 26 Nov 2017 19:32:47 -0500 Subject: [PATCH] python: Fix memleak in do_start_initialization While playing with valgrind, I noticed that with Python 3, the progname variable in do_start_initialization is not being freed (concat returns a malloc'ed string). This patch uses unique_xmalloc_ptr to manage it. With Python 2, we pass progname it directly to Py_SetProgramName, so it should not be freed. We therefore release it before passing it. gdb/ChangeLog: * python/python.c (do_start_initialization): Change progname type to gdb::unique_xmalloc_ptr. Release the pointer when using Python 2. --- gdb/ChangeLog | 6 ++++++ gdb/python/python.c | 12 ++++++------ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index a901409228..b3032ed7d8 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,9 @@ +2017-11-26 Simon Marchi <simon.marchi@polymtl.ca> + + * python/python.c (do_start_initialization): Change progname + type to gdb::unique_xmalloc_ptr. Release the pointer when using + Python 2. + 2017-11-26 Tom Tromey <tom@tromey.com> * common/format.h: Add include guards. diff --git a/gdb/python/python.c b/gdb/python/python.c index 5f152611e8..9ed9b6b1ca 100644 --- a/gdb/python/python.c +++ b/gdb/python/python.c @@ -1658,7 +1658,6 @@ finalize_python (void *ignore) static bool do_start_initialization () { - char *progname; #ifdef IS_PY3K int i; size_t progsize, count; @@ -1672,19 +1671,20 @@ do_start_initialization () /foo/bin/python /foo/lib/pythonX.Y/... This must be done before calling Py_Initialize. */ - progname = concat (ldirname (python_libdir).c_str (), SLASH_STRING, "bin", - SLASH_STRING, "python", (char *) NULL); + gdb::unique_xmalloc_ptr<char> progname + (concat (ldirname (python_libdir).c_str (), SLASH_STRING, "bin", + SLASH_STRING, "python", (char *) NULL)); #ifdef IS_PY3K std::string oldloc = setlocale (LC_ALL, NULL); setlocale (LC_ALL, ""); - progsize = strlen (progname); + progsize = strlen (progname.get ()); progname_copy = (wchar_t *) PyMem_Malloc ((progsize + 1) * sizeof (wchar_t)); if (!progname_copy) { fprintf (stderr, "out of memory\n"); return false; } - count = mbstowcs (progname_copy, progname, progsize + 1); + count = mbstowcs (progname_copy, progname.get (), progsize + 1); if (count == (size_t) -1) { fprintf (stderr, "Could not convert python path to string\n"); @@ -1697,7 +1697,7 @@ do_start_initialization () it is not freed after this call. */ Py_SetProgramName (progname_copy); #else - Py_SetProgramName (progname); + Py_SetProgramName (progname.release ()); #endif #endif -- 2.15.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-11-27 0:36 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-26 3:53 [PATCH] python: Fix memleak in do_start_initialization Simon Marchi 2017-11-26 19:00 ` Tom Tromey 2017-11-26 20:56 ` Simon Marchi 2017-11-26 21:55 ` Tom Tromey 2017-11-27 0:36 ` Simon Marchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox