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 <eholyat@olf.com> 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