Return-Path: Delivered-To: apmail-apr-dev-archive@www.apache.org Received: (qmail 72847 invoked from network); 22 Apr 2010 23:30:45 -0000 Received: from unknown (HELO mail.apache.org) (140.211.11.3) by 140.211.11.9 with SMTP; 22 Apr 2010 23:30:45 -0000 Received: (qmail 14662 invoked by uid 500); 22 Apr 2010 23:30:45 -0000 Delivered-To: apmail-apr-dev-archive@apr.apache.org Received: (qmail 14567 invoked by uid 500); 22 Apr 2010 23:30:45 -0000 Mailing-List: contact dev-help@apr.apache.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Id: Delivered-To: mailing list dev@apr.apache.org Received: (qmail 14560 invoked by uid 99); 22 Apr 2010 23:30:45 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 22 Apr 2010 23:30:45 +0000 X-ASF-Spam-Status: No, hits=2.9 required=10.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_NONE,SPF_NEUTRAL X-Spam-Check-By: apache.org Received-SPF: neutral (nike.apache.org: local policy) Received: from [209.85.221.178] (HELO mail-qy0-f178.google.com) (209.85.221.178) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 22 Apr 2010 23:30:38 +0000 Received: by qyk8 with SMTP id 8so11350140qyk.4 for ; Thu, 22 Apr 2010 16:30:16 -0700 (PDT) MIME-Version: 1.0 Received: by 10.229.86.212 with HTTP; Thu, 22 Apr 2010 16:30:15 -0700 (PDT) In-Reply-To: <648AFB5742D6394FB956DC606975560559CA04C5@OLFANDEXCH01.andover.olf.com> References: <05D9D8EB-F320-4C9B-BC9A-EEC27401C94D@sharp.fm> <648AFB5742D6394FB956DC606975560559CA04C5@OLFANDEXCH01.andover.olf.com> Date: Thu, 22 Apr 2010 19:30:15 -0400 Received: by 10.229.228.83 with SMTP id jd19mr7499506qcb.72.1271979015844; Thu, 22 Apr 2010 16:30:15 -0700 (PDT) Message-ID: Subject: Re: apr_env_set use of putenv From: Wes Garland To: Ed Holyat Cc: Graham Leggett , Dan Poirier , "dev@apr.apache.org" Content-Type: multipart/alternative; boundary=00163630f40f66b5e90484dbb188 X-Virus-Checked: Checked by ClamAV on apache.org --00163630f40f66b5e90484dbb188 Content-Type: text/plain; charset=ISO-8859-1 Leaking is nasty, but I have to agree with Ed here. The problem with Graham's "proper" solution is that it is quite legitimate for a pure-C caller -- e.g. a DSO with no knowledge of APR whatsoever -- to pull an environment variable with getenv and expect to be able to hold on to that pointer for the lifetime of the program. Awful, but true. API fail! Here's what the BSD folks have to say, courtesy of my Leopard box: BUGS Successive calls to setenv() or putenv() assigning a differently sized value to the same name will result in a memory leak. The FreeBSD semantics for these functions (namely, that the contents of value are copied and that old values remain accessible indefi- nitely) make this bug unavoidable. Future versions may eliminate one or both of these semantic guarantees in order to fix the bug. It's pretty much the same everywhere else IME. Wes On Thu, Apr 22, 2010 at 7:18 PM, Ed Holyat wrote: > - Windows automatically copies and controls the memory with _putenv, > allocating a copy on Windows should not be done. > > - Unix requires a memory allocation, you can free the memory in the > environment if you clear the environment variable. > > e.g. UNIX > > mymem=strdup("FOO=abc"); > putenv(mymem); > putenv("FOO="); > free(mymem); > > In my opinion it is not worth the coding effort to keep track of the > environment variables you set and then freeing them. Environment variables > should be set once and used for the life of the program they are not meant > to be set and unset over and over again. > UNIX should just be putenv(strdup("FOO=abc"));//set it and forget it. > > -----Original Message----- > From: Graham Leggett [mailto:minfrin@sharp.fm] > Sent: Monday, March 29, 2010 9:55 AM > To: Dan Poirier > Cc: dev@apr.apache.org > Subject: Re: apr_env_set use of putenv > > On 29 Mar 2010, at 3:36 PM, Dan Poirier wrote: > > > I don't think that's the right pattern to follow. apr_table is used > > to > > allocate a new data structure, owned by the caller, and the caller > > certainly should control its lifetime. apr_env_set() is used to add > > an > > entry to the OS's environment, which the caller does not own and would > > not expect to have any control over the lifetime of its entries. > > From what I can see of the code right now, the caller is expected to > control the lifetime of the string that it passes, or set up their own > cleanup as appropriate to ensure that the environment entry is removed > if the pool is removed. > > The strdup() is by definition a leak, so that isn't ideal at all. > > I suspect the docs would need to be updated to warn the caller than if > they set a string in the environment, they are required to ensure > their string lives as long as the process, or to register their own > cleanup if not. > > Regards, > Graham > -- > > > -- Wesley W. Garland Director, Product Development PageMail, Inc. +1 613 542 2787 x 102 --00163630f40f66b5e90484dbb188 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Leaking is nasty, but I have to agree with Ed here.

The problem with= Graham's "proper" solution is that it is quite legitimate fo= r a pure-C caller -- e.g. a DSO with no knowledge of APR whatsoever -- to p= ull an environment variable with getenv and expect to be able to hold on to= that pointer for the lifetime of the program.

Awful, but true. API fail!

Here's what the BSD folks have to= say, courtesy of my Leopard box:
BUGS
=A0=A0=A0=A0 Successive calls = to setenv() or putenv() assigning a differently sized value to the same nam= e will result in a memory leak.=A0 The
=A0=A0=A0=A0 FreeBSD semantics for these functions (namely, that the conten= ts of value are copied and that old values remain accessible indefi-
=A0= =A0=A0=A0 nitely) make this bug unavoidable.=A0 Future versions may elimina= te one or both of these semantic guarantees in order to fix the
=A0=A0=A0=A0 bug.

It's pretty much the same everywhere else IME.=

Wes

On Thu, Apr 22, 2010 at 7:18 = PM, Ed Holyat <ehol= yat@olf.com> wrote:
- Windows automat= ically copies and controls the memory with _putenv, allocating a copy on Wi= ndows should not be done.

- Unix requires a memory allocation, you can free the memory in the environ= ment if you clear the environment variable.

e.g. =A0UNIX

mymem=3Dstrdup("FOO=3Dabc");
putenv(mymem);
putenv("FOO=3D");
free(mymem);

In my opinion it is not worth the coding effort to keep track of the enviro= nment variables you set and then freeing them. =A0Environment variables sho= uld be set once and used for the life of the program they are not meant to = be set and unset over and over again.
UNIX should just be putenv(strdup("FOO=3Dabc"));//set it and forg= et it.

-----Original Message-----
From: Graham Leggett [mailto:minfrin@sh= arp.fm]
Sent: Monday, March 29, 2010 9:55 AM
To: Dan Poirier
Cc: dev@apr.apache.org
Subject: Re: apr_env_set use of putenv

On 29 Mar 2010, at 3:36 PM, Dan Poirier wrote:

> I don't think that's the right pattern to follow. =A0apr_table= is used
> to
> allocate a new data structure, owned by the caller, and the caller
> certainly should control its lifetime. =A0apr_env_set() is used to add=
> an
> entry to the OS's environment, which the caller does not own and w= ould
> not expect to have any control over the lifetime of its entries.

=A0From what I can see of the code right now, the caller is expected to
control the lifetime of the string that it passes, or set up their own
cleanup as appropriate to ensure that the environment entry is removed
if the pool is removed.

The strdup() is by definition a leak, so that isn't ideal at all.

I suspect the docs would need to be updated to warn the caller than if
they set a string in the environment, they are required to ensure
their string lives as long as the process, or to register their own
cleanup if not.

Regards,
Graham
--





--
Wesley W. G= arland
Director, Product Development
PageMail, Inc.
+1 613 542 278= 7 x 102
--00163630f40f66b5e90484dbb188--