[PATCH] fix double free in misc/win32/apr_app.c and start.c

[Stefan Hett <st...@egosoft.com>]

Hi,

the attached patch fixes a double free issue and a related 
crash/exception when running testapp.exe.

This is the issue Gregg ran into in 2010 during the 1.4.1/1.4.2 release 
voting:
http://www.mail-archive.com/dev%40apr.apache.org/msg22506.html
http://www.mail-archive.com/dev%40apr.apache.org/msg22710.html

I've verified the applied patch fixes the test (testapp.exe) in the 
current 1.5.x branch as well as in 1.5.2. As far as I can see the issue 
is present in all versions (dating back to/including the 0.9.x branch - 
corresponding code change was introduced in 2002 in).

I take it that the current implementation relied on implementation 
details specific to the Visual Studio version in 2002. As far as I can 
tell, the issue can at least be triggered with VS >= 2010 (Gregg's mail 
suggests he ran into the problem with VS 2008 - so I take it the problem 
could have been introduced with VS 2005 or maybe even with VS.Net).

The issue in detail:
The corresponding code in apr/misc/win32/apr_app.c: wmain() (and 
related: start.c: apr_app_initialize()) takes the given environment, 
converts it from the passed wchar environment into the UTF-8 style, sets 
the _environ-variable with the converted data and clears the 
_wenviron-variable, so that the application can rely on the data being 
available in a consistent UTF-8 format (platform independent).

The problem with freeing the _wenviron variable after having it set to 
NULL is that the current implementation in the VS runtime keeps a copy 
of the original _wenviron-pointer which it cleans up in the end. So even 
when setting _wenviron-data to NULL, quitting the application still 
tries to free the memory (which was freed already before), resulting in 
the crash/exception (and in theory in undefined behavior).
Under the assumption that this was not the behavior for some older 
versions of VS, I understand this was changed (by MS) to properly free 
up the allocated memory (since one would take it being the 
responsibility of the VS runtime to free what it actually allocates).

The attached patch simply drops the explicit free-call for the old 
_wenviron (and leaves it with the implicit handling in the VS runtime to 
clear this in the end). Since I don't have an older VS compiler at hand 
(aka: pre-VS 2010) I can't easily verify whether the patch results in 
correct behavior for older versions of Visual Studio; but as far as I 
can tell there shouldn't be any problem. The only behavior change I 
expect compared to the old code is that with this fix apps take up a few 
more bytes (maybe a few kb) of memory, since the _wenviron-data is not 
freed at startup.

TBH I'm not fully satisfied with the quality of the patch myself, but 
would still propose it for APR 1.5.x because of:
- fixes a crash/exception
- is a minimal code change to resolve the crash/exception

IMO a better approach would be to stop tweaking the internal 
_environ/_wenviron data directly, but rather rely on the provided API to 
perform the required operations. In particular an alternative approach 
could go like this:
1. Get a copy of the current environment variables 
(GetEnvironmentStringsW())
2. Iterate over the retrieved elements and call _wputenv("var=") for 
each entry to unset the variable
3. For each converted environment variable call _putenv("var=value")

I believe the result of that approach being the same as what's intended 
with the current code (and also already tested that approach proofing 
all tests pass with that one too - including that fixing the 
crash/exception in testapp.exe).
Still, I'm proposing the other code change at least for the 1.5-branch 
since that code change is far less than the other approach, and not 
being familiar with all the details of APR I might overlook some 
important fact which would break with the other approach.
Obviously, the advantage of the putenv-approach is that this is more 
portable for future VS versions, especially since the _environ/_wenviron 
variables have been deprecated since VS 2005 and hence might get dropped 
in some future version 
(https://msdn.microsoft.com/en-us/library/stxk41x1(v=vs.80).aspx).

-- 
Regards,
Stefan Hett