From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11681 invoked by alias); 8 Aug 2007 21:58:08 -0000 Received: (qmail 11537 invoked by uid 22791); 8 Aug 2007 21:58:08 -0000 X-Spam-Check-By: sourceware.org Received: from b.mail.sonic.net (HELO b.mail.sonic.net) (64.142.19.5) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 08 Aug 2007 21:57:59 +0000 Received: from webmail.sonic.net (b.webmail.sonic.net [64.142.100.148]) by b.mail.sonic.net (8.13.8.Beta0-Sonic/8.13.7) with ESMTP id l78LvvWT005584; Wed, 8 Aug 2007 14:57:57 -0700 Received: from 12.7.175.2 (SquirrelMail authenticated user msnyder) by webmail.sonic.net with HTTP; Wed, 8 Aug 2007 14:57:57 -0700 (PDT) Message-ID: <22684.12.7.175.2.1186610277.squirrel@webmail.sonic.net> In-Reply-To: References: <21906.12.7.175.2.1186283848.squirrel@webmail.sonic.net> <18682.12.7.175.2.1186597784.squirrel@webmail.sonic.net> Date: Wed, 08 Aug 2007 21:58:00 -0000 Subject: Re: [PATCH] solib_open, memory leak From: msnyder@sonic.net To: "Jim Blandy" Cc: gdb-patches@sourceware.org User-Agent: SquirrelMail/1.4.9a MIME-Version: 1.0 Content-Type: multipart/mixed;boundary="----=_20070808145757_62205" 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/msg00163.txt.bz2 ------=_20070808145757_62205 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 8bit Content-length: 699 [Jim:] > The code clearly deserves a comment like: > > /* We try to find the library in various ways. After each > attempt, either found_file >= 0 and temp_pathname is a malloc'd > string, or found_file < 0 and temp_pathname does not point to > storage that needs to be freed. */ > > In that light, it seems clearer to me to just put: > > if (found_file < 0) > temp_pathname = NULL; > else > temp_pathname = xstrdup (temp_pathname); > > immediately after the open, to make it obvious that the rule is > followed there. > [...] > Don't you want those new lines to replace the two that followed them, > not just precede them? Yes, thanks. How about this? ------=_20070808145757_62205 Content-Type: text/plain; name="solib_open2.txt" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="solib_open2.txt" Content-length: 1692 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 21:57:27 -0000 *************** solib_open (char *in_pathname, char **fo *** 176,181 **** --- 176,192 ---- /* Now see if we can open it. */ found_file = open (temp_pathname, O_RDONLY | O_BINARY, 0); + /* We try to find the library in various ways. After each attempt + (except for the one above), either found_file >= 0 and + temp_pathname is a malloc'd string, or found_file < 0 and + temp_pathname does not point to storage that needs to be + freed. */ + + if (found_file < 0) + temp_pathname = NULL; + else + temp_pathname = xstrdup (temp_pathname); + /* 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.) *************** solib_open (char *in_pathname, char **fo *** 224,231 **** /* Done. If not found, tough luck. Return found_file and (optionally) found_pathname. */ ! if (found_pathname != NULL && temp_pathname != NULL) ! *found_pathname = xstrdup (temp_pathname); return found_file; } --- 235,247 ---- /* Done. If not found, tough luck. Return found_file and (optionally) found_pathname. */ ! if (temp_pathname) ! { ! if (found_pathname != NULL) ! *found_pathname = temp_pathname; ! else ! xfree (temp_pathname); ! } return found_file; } ------=_20070808145757_62205--