From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13419 invoked by alias); 8 Aug 2007 18:29:53 -0000 Received: (qmail 13329 invoked by uid 22791); 8 Aug 2007 18:29:52 -0000 X-Spam-Check-By: sourceware.org Received: from a.mail.sonic.net (HELO a.mail.sonic.net) (64.142.16.245) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 08 Aug 2007 18:29:50 +0000 Received: from webmail.sonic.net (b.webmail.sonic.net [64.142.100.148]) by a.mail.sonic.net (8.13.8.Beta0-Sonic/8.13.7) with ESMTP id l78ITil8001078; Wed, 8 Aug 2007 11:29:44 -0700 Received: from 12.7.175.2 (SquirrelMail authenticated user msnyder) by webmail.sonic.net with HTTP; Wed, 8 Aug 2007 11:29:44 -0700 (PDT) Message-ID: <18682.12.7.175.2.1186597784.squirrel@webmail.sonic.net> In-Reply-To: References: <21906.12.7.175.2.1186283848.squirrel@webmail.sonic.net> Date: Wed, 08 Aug 2007 18:29:00 -0000 Subject: Re: [PATCH] solib_open, memory leak From: msnyder@sonic.net To: "Jim Blandy" Cc: msnyder@sonic.net, gdb-patches@sourceware.org User-Agent: SquirrelMail/1.4.9a MIME-Version: 1.0 Content-Type: multipart/mixed;boundary="----=_20070808112944_88833" Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2007-08/txt/msg00152.txt.bz2 ------=_20070808112944_88833 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 8bit Content-length: 1498 > > msnyder@sonic.net writes: >> I hope it's not getting to be too late at night for me to do this >> stuff... >> >> If I'm not spacy, solib_open is leaking memory, because openp passes >> back >> a malloc buffer for temp_pathname. In order to free it, it has to >> always >> be a malloc buffer (hence no alloca etc). > > I think you were a little bit spacy. :) > > Even if you've made sure that temp_pathname is malloc'd by the time we > reach the 'open', any later 'openp' call will throw away its value. > openp is careful to store *something* in the pointer referred to by > its last argument, even on error. Uhhh, yeah, you're right. > I think the invariant should be that, when found_file becomes >= 0, > then temp_pathname is malloc'd, and not before. The 'openp' clauses > will preserve that. So I think you need: > > if (found_file >= 0) > temp_pathname = xstrdup (temp_pathname); > > after the 'open'. > > And then there's no need for the xstrdup at the bottom; just return > temp_pathname, or xfree it if the caller doesn't want it. That sounds great, except for one thing (which I had also not addressed). If openp receives an xmalloc'd temp_pathname, it will clobber it without freeing it. I'm thinking that openp will never use the buffer, only the pointer. So rather than what you suggest, what if we AVOID xmalloc before openp and just make sure that we pass openp a null pointer or a pointer to memory that does not need to be freed (such as alloca). Umm, thus: ------=_20070808112944_88833 Content-Type: text/plain; name="solib_open.txt" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="solib_open.txt" Content-length: 2664 Index: solib.c =================================================================== RCS file: /cvs/src/src/gdb/solib.c,v retrieving revision 1.94 diff -p -r1.94 solib.c *** solib.c 3 Jul 2007 12:14:43 -0000 1.94 --- solib.c 8 Aug 2007 18:29:15 -0000 *************** solib_open (char *in_pathname, char **fo *** 176,194 **** /* Now see if we can open it. */ found_file = open (temp_pathname, O_RDONLY | O_BINARY, 0); ! /* If the search in gdb_sysroot failed, and the path name is ! absolute at this point, make it relative. (openp will try and open the ! file according to its absolute path otherwise, which is not what we want.) ! Affects subsequent searches for this solib. */ ! if (found_file < 0 && IS_ABSOLUTE_PATH (in_pathname)) ! { ! /* First, get rid of any drive letters etc. */ ! while (!IS_DIR_SEPARATOR (*in_pathname)) ! in_pathname++; ! ! /* Next, get rid of all leading dir separators. */ ! while (IS_DIR_SEPARATOR (*in_pathname)) ! in_pathname++; } /* If not found, search the solib_search_path (if any). */ --- 176,205 ---- /* Now see if we can open it. */ found_file = open (temp_pathname, O_RDONLY | O_BINARY, 0); ! if (found_file < 0) ! { ! /* If the search in gdb_sysroot failed, and the path name is ! absolute at this point, make it relative. (openp will try ! and open the file according to its absolute path otherwise, ! which is not what we want.) Affects subsequent searches for ! this solib. */ ! if (IS_ABSOLUTE_PATH (in_pathname)) ! { ! /* First, get rid of any drive letters etc. */ ! while (!IS_DIR_SEPARATOR (*in_pathname)) ! in_pathname++; ! ! /* Next, get rid of all leading dir separators. */ ! while (IS_DIR_SEPARATOR (*in_pathname)) ! in_pathname++; ! } ! /* Previous temp_pathname buffer is no longer in use. */ ! temp_pathname = NULL; ! } ! else if (temp_pathname) ! { ! /* Make a copy that can be returned. */ ! temp_pathname = xstrdup (temp_pathname); } /* If not found, search the solib_search_path (if any). */ *************** solib_open (char *in_pathname, char **fo *** 224,229 **** --- 235,247 ---- /* Done. If not found, tough luck. Return found_file and (optionally) found_pathname. */ + if (temp_pathname) + { + if (found_pathname) + *found_pathname = temp_pathname; + else + xfree (temp_pathname); + } if (found_pathname != NULL && temp_pathname != NULL) *found_pathname = xstrdup (temp_pathname); return found_file; ------=_20070808112944_88833--