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
Index: misc/win32/apr_app.c
===================================================================
--- misc/win32/apr_app.c (revision 1748371)
+++ misc/win32/apr_app.c (working copy)
@@ -71,7 +71,6 @@
if (_wenviron) {
wenv = _wenviron;
_wenviron = NULL;
- free((wchar_t **)wenv);
}
apr_app_init_complete = 1;
Index: misc/win32/start.c
===================================================================
--- misc/win32/start.c (revision 1748371)
+++ misc/win32/start.c (working copy)
@@ -156,7 +156,6 @@
if (_wenviron) {
apr_wchar_t **wenv = _wenviron;
_wenviron = NULL;
- free(wenv);
}
}
