From owner-freebsd-current@FreeBSD.ORG Thu Oct 19 17:27:21 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 48AB616A47E for ; Thu, 19 Oct 2006 17:27:21 +0000 (UTC) (envelope-from se@freebsd.org) Received: from mail.atsec.com (mail.atsec.com [195.30.252.105]) by mx1.FreeBSD.org (Postfix) with ESMTP id 60BC343D7D for ; Thu, 19 Oct 2006 17:26:31 +0000 (GMT) (envelope-from se@freebsd.org) Received: (qmail 29761 invoked by uid 10125); 19 Oct 2006 17:26:30 -0000 X-SpaceNet-Virusscan: Sophos Version: 4.09; Last IDE Update: 2006-10-19 14:40 no information about results Received: from unknown (HELO ?10.254.1.25?) (195.30.9.200) by mail.atsec.com with SMTP; 19 Oct 2006 17:26:30 -0000 X-SpaceNet-Authentification: SMTP AUTH verified Message-ID: <4537B53A.3080208@FreeBSD.org> Date: Thu, 19 Oct 2006 19:26:18 +0200 From: Stefan Esser User-Agent: Thunderbird 1.5.0.7 (Windows/20060909) MIME-Version: 1.0 To: "Sean C. Farley" References: <20061006200320.T1063@baba.farley.org> <200610111427.42195.jhb@freebsd.org> <20061018211005.L1466@baba.farley.org> <200610191032.29232.jhb@freebsd.org> <20061019112601.J91957@thor.farley.org> In-Reply-To: <20061019112601.J91957@thor.farley.org> X-Enigmail-Version: 0.94.1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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 17:27:21 -0000 Sean C. Farley wrote: > On Thu, 19 Oct 2006, John Baldwin wrote: [...] >> 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 Hmmm, I assume you meant: w == y ("BAR2") and x == z ("BARBAR2") ... > 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. Just curious, why don't you ignore the first slot allocated to "BAR1" during the setenv("FOO", "BAR2", 1) and overwrite the value at *x ... I.e.: w ("BAR1") and x == y == z ("BARBAR2") ... If the maximum size is recorded, there is no reason to prefer *w over *x, and the case of a cached pointer might still be wrong but not that irritating once the variable has been set to its longest value. I've got to admit, that I have not looked your patch, but the only drawback seems to be that the last instance of a variable in the environment space has to be located in getenv() (maximizing the search time ...). Always using the last allocated (largest) slot for storage of new values of environment variables would result in nearly reasonable behavior. A cached pointer does either point to the value of the variable at the time of the getenv(), or to the last value assigned to the environment variable that does not exceed the allocated size. Regards, STefan