From owner-freebsd-current@FreeBSD.ORG Thu Oct 19 18:01:44 2006 Return-Path: X-Original-To: freebsd-current@freebsd.org Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 0E55E16A4E1 for ; Thu, 19 Oct 2006 18:01:44 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from server.baldwin.cx (66-23-211-162.clients.speedfactory.net [66.23.211.162]) by mx1.FreeBSD.org (Postfix) with ESMTP id 4C9BA43D6A for ; Thu, 19 Oct 2006 18:01:34 +0000 (GMT) (envelope-from jhb@freebsd.org) Received: from localhost.corp.yahoo.com (john@localhost [127.0.0.1]) (authenticated bits=0) by server.baldwin.cx (8.13.6/8.13.6) with ESMTP id k9JI1JFp067884; Thu, 19 Oct 2006 14:01:19 -0400 (EDT) (envelope-from jhb@freebsd.org) From: John Baldwin To: "Sean C. Farley" Date: Thu, 19 Oct 2006 13:10:04 -0400 User-Agent: KMail/1.9.1 References: <20061006200320.T1063@baba.farley.org> <200610191032.29232.jhb@freebsd.org> <20061019112601.J91957@thor.farley.org> In-Reply-To: <20061019112601.J91957@thor.farley.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200610191310.05604.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [127.0.0.1]); Thu, 19 Oct 2006 14:01:19 -0400 (EDT) X-Virus-Scanned: ClamAV 0.88.3/2050/Thu Oct 19 03:58:33 2006 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=4.2 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.1.3 X-Spam-Checker-Version: SpamAssassin 3.1.3 (2006-06-01) on server.baldwin.cx Cc: freebsd-current@freebsd.org Subject: Re: Fix for memory leak in setenv/unsetenv X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Oct 2006 18:01:44 -0000 On Thursday 19 October 2006 12:54, Sean C. Farley wrote: > On Thu, 19 Oct 2006, John Baldwin wrote: > > > On Wednesday 18 October 2006 22:50, Sean C. Farley wrote: > >> On Wed, 11 Oct 2006, John Baldwin wrote: > >>> On Wednesday 11 October 2006 12:15, Sean C. Farley wrote: > >>>> On Tue, 10 Oct 2006, John Baldwin wrote: > >>>>> This still won't work. The reason for the intentional leak is > >>>>> because of this code sequence: > >>>>> > >>>>> char *a; > >>>>> > >>>>> setenv("FOO", "0", 1); > >>>>> a = getenv("FOO"); > >>>>> setenv("FOO", "bar", 1); > >>>>> printf("FOO was %s\n", a); > >>>>> > >>>>> With the memory leak fixed this will use free'd memory. While > >>>>> this code may seem weird in a program, it actually is quite > >>>>> possible for a library to read and cache the value of an > >>>>> environment variable. If you didn't leave the leak around, the > >>>>> library could cause a crash if the main program (or another > >>>>> library) changed the environment variable the first library had a > >>>>> cached pointer to the value of. > >> > >> > >> > >>> Yeah, but it doesn't crash is the point actually. The pointer is > >>> still valid, though it may be overwritten with a newer value, it's > >>> still valid and a library can reliably doing getenv() and that > >>> pointer will always point to some value of that variable, but it > >>> won't ever point to anything else. > >> > >> > >> > >>> Part of the problem is that we have no way to notify consumers of an > >>> environment variable when its value is changed. Alternatively, we > >>> could add a different variant of getenv that required the user to > >>> supply the buffer, but that's not the API we have. > >> > >> OK. I decided to fix the memory leak as well as keep backward > >> compatibility. The result is on my site tar'd[1] and extracted[2]. > >> It still needs some touch-ups, but it works. It is even faster than > >> the current implementation when I compared "hungry" and "lean" > >> (main.c without the sleep() call). > > > > I don't see how you fixed the leak. You explicitly mention that you > > don't free old values, so you are preserving the leak. > > I preserve the leak, but I also make use of old entries with matching > names if their size is big enough. The maximum size of the value is > recorded at allocation. The code in the source tree uses the strlen() > of the current value which can shrink even if the space is available. > > Example: > setenv("FOO", "BAR1", 1); > w = getenv("FOO"); > setenv("FOO", "BARBAR1", 1); > x = getenv("FOO"); > setenv("FOO", "BAR2", 1); > y = getenv("FOO"); > setenv("FOO", "BARBAR2", 1); > z = getenv("FOO"); > > This will end up with w == y ("BAR2") and y == z ("BAR1"). w was > allocated first in the array of variables. x is then allocated since w > is too small. y finds w within the array and reuses it. z does the > wame with x. If the larger value had been allocated first, then all > setenv() calls would have used the same storage space. Yes, there is a > leak, but flipping back and forth does not cause the leak to keep > growing. Also, there would be no need to space-pad a value to prevent > the leak. Ah, very cool. -- John Baldwin