Message ID | 20170322131132.98976-1-prudo@linux.vnet.ibm.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 55293 invoked by alias); 22 Mar 2017 13:11:40 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 55263 invoked by uid 89); 22 Mar 2017 13:11:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy= X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0b-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.158.5) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 22 Mar 2017 13:11:38 +0000 Received: from pps.filterd (m0098414.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v2MD4K6G028532 for <gdb-patches@sourceware.org>; Wed, 22 Mar 2017 09:11:37 -0400 Received: from e06smtp11.uk.ibm.com (e06smtp11.uk.ibm.com [195.75.94.107]) by mx0b-001b2d01.pphosted.com with ESMTP id 29b9vqmfnm-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for <gdb-patches@sourceware.org>; Wed, 22 Mar 2017 09:11:37 -0400 Received: from localhost by e06smtp11.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for <gdb-patches@sourceware.org> from <prudo@linux.vnet.ibm.com>; Wed, 22 Mar 2017 13:11:35 -0000 Received: from b06cxnps4076.portsmouth.uk.ibm.com (9.149.109.198) by e06smtp11.uk.ibm.com (192.168.101.141) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 22 Mar 2017 13:11:33 -0000 Received: from d06av26.portsmouth.uk.ibm.com (d06av26.portsmouth.uk.ibm.com [9.149.105.62]) by b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v2MDBWI639321672 for <gdb-patches@sourceware.org>; Wed, 22 Mar 2017 13:11:32 GMT Received: from d06av26.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id ED2FEAE04D for <gdb-patches@sourceware.org>; Wed, 22 Mar 2017 13:11:03 +0000 (GMT) Received: from d06av26.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id D837DAE058 for <gdb-patches@sourceware.org>; Wed, 22 Mar 2017 13:11:03 +0000 (GMT) Received: from tuxmaker.boeblingen.de.ibm.com (unknown [9.152.85.9]) by d06av26.portsmouth.uk.ibm.com (Postfix) with ESMTPS for <gdb-patches@sourceware.org>; Wed, 22 Mar 2017 13:11:03 +0000 (GMT) From: Philipp Rudo <prudo@linux.vnet.ibm.com> To: gdb-patches@sourceware.org Subject: [PATCH] Fix memory leak in python.c:do_start_initialization Date: Wed, 22 Mar 2017 14:11:31 +0100 X-TM-AS-GCONF: 00 x-cbid: 17032213-0040-0000-0000-0000036BC377 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17032213-0041-0000-0000-00001F5FD88C Message-Id: <20170322131132.98976-1-prudo@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2017-03-22_11:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=1 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1702020001 definitions=main-1703220115 X-IsSubscribed: yes |
Commit Message
Philipp Rudo
March 22, 2017, 1:11 p.m. UTC
When intializing Python the path to the python binary is build the following way progname = concat (ldirname (python_libdir), SLASH_STRING, "bin", SLASH_STRING, "python", (char *) NULL); This is problematic as both concat and ldirname allocate memory for the string they return. Thus the memory allocated by ldirname cannot be accessed afterwards causing a memory leak. Fix it by temporarily storing libdir in a variable and xfree it after concat. gdb/ChangeLog: python/python.c (do_start_initialization): Fix memory leak. --- gdb/python/python.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Comments
Hi Philipp, Thanks. On 03/22/2017 01:11 PM, Philipp Rudo wrote: > diff --git a/gdb/python/python.c b/gdb/python/python.c > index 73fb3d0..6b16613 100644 > --- a/gdb/python/python.c > +++ b/gdb/python/python.c > @@ -1535,7 +1535,7 @@ extern initialize_file_ftype _initialize_python; > static bool > do_start_initialization () > { > - char *progname; > + char *progname, *libdir; > #ifdef IS_PY3K > int i; > size_t progsize, count; > @@ -1550,8 +1550,10 @@ do_start_initialization () > /foo/bin/python > /foo/lib/pythonX.Y/... > This must be done before calling Py_Initialize. */ > - progname = concat (ldirname (python_libdir), SLASH_STRING, "bin", > + libdir = ldirname (python_libdir); > + progname = concat (libdir, SLASH_STRING, "bin", > SLASH_STRING, "python", (char *) NULL); > + xfree (libdir); Let's restrict the new variable to the #if block that needs it. I.e., declare the variable where is initialized, like: const char *libdir = ldirname (python_libdir); progname = concat (libdir, SLASH_STRING, "bin", OK with that change. Please push. Note, you could have used reconcat instead of concat, avoiding the xfree call, and maybe one reallocation, but that's hardly an issue here. Perhaps better overall would be to make ldirname return a std::string and eliminate these leaks "by design". It'd get rid of several make_cleanup calls throughout too. I'll give that a quick try. Thanks, Pedro Alves
On Wed, 22 Mar 2017 15:19:16 +0000 Pedro Alves <palves@redhat.com> wrote: > Hi Philipp, > > Thanks. > > On 03/22/2017 01:11 PM, Philipp Rudo wrote: > > > diff --git a/gdb/python/python.c b/gdb/python/python.c > > index 73fb3d0..6b16613 100644 > > --- a/gdb/python/python.c > > +++ b/gdb/python/python.c > > @@ -1535,7 +1535,7 @@ extern initialize_file_ftype > > _initialize_python; static bool > > do_start_initialization () > > { > > - char *progname; > > + char *progname, *libdir; > > #ifdef IS_PY3K > > int i; > > size_t progsize, count; > > @@ -1550,8 +1550,10 @@ do_start_initialization () > > /foo/bin/python > > /foo/lib/pythonX.Y/... > > This must be done before calling Py_Initialize. */ > > - progname = concat (ldirname (python_libdir), SLASH_STRING, "bin", > > + libdir = ldirname (python_libdir); > > + progname = concat (libdir, SLASH_STRING, "bin", > > SLASH_STRING, "python", (char *) NULL); > > + xfree (libdir); > > Let's restrict the new variable to the #if block that needs it. > I.e., declare the variable where is initialized, like: > > const char *libdir = ldirname (python_libdir); > progname = concat (libdir, SLASH_STRING, "bin", > > OK with that change. Please push. Shall I fix it? Or do we go with your ldirname fix? > > Note, you could have used reconcat instead of concat, avoiding the > xfree call, and maybe one reallocation, but that's hardly an > issue here. Thought about using reconcat. But in the end reconcat would have done the same -- calling free on libdir. Thats why I decided it would be better readable when not hidden. > Perhaps better overall would be to make ldirname return a std::string > and eliminate these leaks "by design". It'd get rid of several > make_cleanup calls throughout too. I'll give that a quick try. Or maybe combining all path handling in a 'class gdbpath'. But make ldirname return std::string definitely is an advantage. > > Thanks, > Pedro Alves >
On 03/22/2017 05:52 PM, Philipp Rudo wrote: >> OK with that change. Please push. > > Shall I fix it? Or do we go with your ldirname fix? Please push in your fix first. It's small and obviously correct. >> Perhaps better overall would be to make ldirname return a std::string >> and eliminate these leaks "by design". It'd get rid of several >> make_cleanup calls throughout too. I'll give that a quick try. > > Or maybe combining all path handling in a 'class gdbpath'. But > make ldirname return std::string definitely is an advantage. That'd be nice. If we do that, I think we should model the API on C++17's filesystem::path http://en.cppreference.com/w/cpp/filesystem/path , to ease a future transition. Thanks, Pedro Alves
diff --git a/gdb/python/python.c b/gdb/python/python.c index 73fb3d0..6b16613 100644 --- a/gdb/python/python.c +++ b/gdb/python/python.c @@ -1535,7 +1535,7 @@ extern initialize_file_ftype _initialize_python; static bool do_start_initialization () { - char *progname; + char *progname, *libdir; #ifdef IS_PY3K int i; size_t progsize, count; @@ -1550,8 +1550,10 @@ do_start_initialization () /foo/bin/python /foo/lib/pythonX.Y/... This must be done before calling Py_Initialize. */ - progname = concat (ldirname (python_libdir), SLASH_STRING, "bin", + libdir = ldirname (python_libdir); + progname = concat (libdir, SLASH_STRING, "bin", SLASH_STRING, "python", (char *) NULL); + xfree (libdir); #ifdef IS_PY3K oldloc = xstrdup (setlocale (LC_ALL, NULL)); setlocale (LC_ALL, "");