* Christian Ullrich wrote:
wrong even without considering the debug/release split. If we load a
compiled extension built with a CRT we have not seen yet, _after_ the
first call to pgwin32_putenv(), that module's CRT's view of its
environment will be frozen because we will never attempt to update it.
Four patches attached:
master --- 0001 --- 0002 --- 0003
\
`- 0004
0001 is just some various fixes to set the stage.
0002 fixes this "load race" by not remembering a negative result
anymore. However, ...
If it can happen that a CRT DLL is unloaded before the process exits,
and we cached the module handle while it was loaded, and later
pgwin32_putenv() is called, that won't end well for the process. This
might be a bit far-fetched; I have to see if I can actually make it happen.
... this *can* and does happen, so fixing the load race alone is not
enough. 0004 fixes the unload race as well, by dropping the entire DLL
handle/_putenv pointer cache from the function and going through the
list of DLLs each time.
I tested this with a project
(<https://bitbucket.org/chrullrich/pgputenv-demo>) that contains two DLLs:
- The first one is built with VS 2013 (debug), as is the server. It
does not matter what it is built with, except it must not be the same
as the second DLL. It exports a single function callable from SQL.
- The second one is built with VS 2015 (debug), and again, the exact
CRT is not important as long as it is not the same as the server
or the first DLL.
The function does this:
1. It loads the second DLL. This brings in ucrtbased.dll as well.
2. It calls putenv().
3. It unloads the second DLL. This also causes ucrtbased.dll to be
unloaded because it is not needed anymore.
4. It calls putenv() again.
- With current master, this works, because pgwin32_putenv(),
after the first call somewhere early during backend startup,
never looks for ucrtbased again (including in step 2).
- With patch 0002 applied, it crashes because pgwin32_putenv(),
having detected ucrtbased.dll and cached its HMODULE during
the call in step 2 above, reuses these values after the DLL
is long gone.
- With patch 0004 applied as well, it works again because no
caching is done anymore.
Even with patch 0004, there is still a race condition between the main
thread and a theoretical additional thread created by some other
component that unloads some CRT between GetProcAddress() and the
_putenv() call, but that is hardly fixable.
The fact that master looks like it does means that there have not been
many (or any) complaints about missing cross-module environment
variables. If nobody ever needs to see a variable set elsewhere, we have
a very simple solution: Why don't we simply throw out the whole #ifdef
_MSC_VER block?
There is another possible fix, ugly as sin, if we really need to keep
the whole environment update machinery *and* cannot do the full loop
each time. Patch 0003 pins each CRT when we see it for the first time.
GET_MODULE_HANDLE_EX_FLAG_PIN is documented as "The module stays loaded
until the process is terminated, no matter how many times FreeLibrary is
called", so the unload race cannot occur anymore.
--
Christian
From d74e778d8abbfea244bacbeb352cadc737032e85 Mon Sep 17 00:00:00 2001
From: Christian Ullrich <ch...@chrullrich.net>
Date: Tue, 26 Apr 2016 15:26:14 +0200
Subject: [PATCH 1/3] Cleanups in pgwin32_putenv().
- Added UCRT and all debug CRTs
- Condensed the array to one line per item (it was getting long)
- Test HMODULEs for NULL instead of zero
- Removed unnecessary CloseHandle() call
---
src/port/win32env.c | 49 ++++++++++++++++++++-----------------------------
1 file changed, 20 insertions(+), 29 deletions(-)
diff --git a/src/port/win32env.c b/src/port/win32env.c
index 7e4ff62..fd6762e 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -45,33 +45,25 @@ pgwin32_putenv(const char *envval)
PUTENVPROC putenvFunc;
} rtmodules[] =
{
- {
- "msvcrt", 0, NULL
- }, /* Visual
Studio 6.0 / mingw */
- {
- "msvcr70", 0, NULL
- }, /* Visual
Studio 2002 */
- {
- "msvcr71", 0, NULL
- }, /* Visual
Studio 2003 */
- {
- "msvcr80", 0, NULL
- }, /* Visual
Studio 2005 */
- {
- "msvcr90", 0, NULL
- }, /* Visual
Studio 2008 */
- {
- "msvcr100", 0, NULL
- }, /* Visual
Studio 2010 */
- {
- "msvcr110", 0, NULL
- }, /* Visual
Studio 2012 */
- {
- "msvcr120", 0, NULL
- }, /* Visual
Studio 2013 */
- {
- NULL, 0, NULL
- }
+ { "msvcrt", NULL, NULL },
+ { "msvcrtd", NULL, NULL }, /* Visual Studio 6.0 / mingw */
+ { "msvcr70", NULL, NULL },
+ { "msvcr70d", NULL, NULL }, /* Visual Studio 2002 */
+ { "msvcr71", NULL, NULL },
+ { "msvcr71d", NULL, NULL }, /* Visual Studio 2003 */
+ { "msvcr80", NULL, NULL },
+ { "msvcr80d", NULL, NULL }, /* Visual Studio 2005 */
+ { "msvcr90", NULL, NULL },
+ { "msvcr90d", NULL, NULL }, /* Visual Studio 2008 */
+ { "msvcr100", NULL, NULL },
+ { "msvcr100d", NULL, NULL }, /* Visual Studio 2010 */
+ { "msvcr110", NULL, NULL },
+ { "msvcr110d", NULL, NULL }, /* Visual Studio 2012 */
+ { "msvcr120", NULL, NULL },
+ { "msvcr120d", NULL, NULL }, /* Visual Studio 2013 */
+ { "ucrtbase", NULL, NULL },
+ { "ucrtbased", NULL, NULL }, /* Visual Studio 2015+ */
+ { NULL, 0, NULL }
};
int i;
@@ -79,7 +71,7 @@ pgwin32_putenv(const char *envval)
{
if (rtmodules[i].putenvFunc == NULL)
{
- if (rtmodules[i].hmodule == 0)
+ if (rtmodules[i].hmodule == NULL)
{
/* Not attempted before, so try to find this
DLL */
rtmodules[i].hmodule =
GetModuleHandle(rtmodules[i].modulename);
@@ -97,7 +89,6 @@ pgwin32_putenv(const char *envval)
rtmodules[i].putenvFunc = (PUTENVPROC)
GetProcAddress(rtmodules[i].hmodule, "_putenv");
if (rtmodules[i].putenvFunc == NULL)
{
-
CloseHandle(rtmodules[i].hmodule);
rtmodules[i].hmodule =
INVALID_HANDLE_VALUE;
continue;
}
--
2.8.1.windows.1
From e1c705b2657dd5b93c4dd2683b9032a656396571 Mon Sep 17 00:00:00 2001
From: Christian Ullrich <ch...@chrullrich.net>
Date: Tue, 26 Apr 2016 15:45:00 +0200
Subject: [PATCH 2/3] Fix the load race in pgwin32_putenv() (and open the
unload race).
Before, any CRT first loaded after the first call to pgwin32_putenv() would
be frozen out because the "not found" result was cached for the lifetime of
the process.
This fixes the "load" race and makes it much more likely that an "unload" race
happens instead, where a CRT is loaded, noticed and cached, then unloaded, and
then the next call to pgwin32_putenv() crashes.
---
src/port/win32env.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/src/port/win32env.c b/src/port/win32env.c
index fd6762e..9afc31f 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -73,15 +73,10 @@ pgwin32_putenv(const char *envval)
{
if (rtmodules[i].hmodule == NULL)
{
- /* Not attempted before, so try to find this
DLL */
+ /* Try to find this DLL */
rtmodules[i].hmodule =
GetModuleHandle(rtmodules[i].modulename);
if (rtmodules[i].hmodule == NULL)
{
- /*
- * Set to INVALID_HANDLE_VALUE so we
know we have tried
- * this one before, and won't try again.
- */
- rtmodules[i].hmodule =
INVALID_HANDLE_VALUE;
continue;
}
else
@@ -89,7 +84,6 @@ pgwin32_putenv(const char *envval)
rtmodules[i].putenvFunc = (PUTENVPROC)
GetProcAddress(rtmodules[i].hmodule, "_putenv");
if (rtmodules[i].putenvFunc == NULL)
{
- rtmodules[i].hmodule =
INVALID_HANDLE_VALUE;
continue;
}
}
--
2.8.1.windows.1
From aa129120b12c4ffd907b8765b30044691696a1b2 Mon Sep 17 00:00:00 2001
From: Christian Ullrich <ch...@chrullrich.net>
Date: Tue, 26 Apr 2016 18:46:17 +0200
Subject: [PATCH 3/3] Pin any DLL as soon as we see it, to avoid the unload
race.
---
src/port/win32env.c | 49 ++++++++++++++++++++++++++++++-------------------
1 file changed, 30 insertions(+), 19 deletions(-)
diff --git a/src/port/win32env.c b/src/port/win32env.c
index 9afc31f..57a9c7d 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -43,27 +43,28 @@ pgwin32_putenv(const char *envval)
char *modulename;
HMODULE hmodule;
PUTENVPROC putenvFunc;
+ bool pinned;
} rtmodules[] =
{
- { "msvcrt", NULL, NULL },
- { "msvcrtd", NULL, NULL }, /* Visual Studio 6.0 / mingw */
- { "msvcr70", NULL, NULL },
- { "msvcr70d", NULL, NULL }, /* Visual Studio 2002 */
- { "msvcr71", NULL, NULL },
- { "msvcr71d", NULL, NULL }, /* Visual Studio 2003 */
- { "msvcr80", NULL, NULL },
- { "msvcr80d", NULL, NULL }, /* Visual Studio 2005 */
- { "msvcr90", NULL, NULL },
- { "msvcr90d", NULL, NULL }, /* Visual Studio 2008 */
- { "msvcr100", NULL, NULL },
- { "msvcr100d", NULL, NULL }, /* Visual Studio 2010 */
- { "msvcr110", NULL, NULL },
- { "msvcr110d", NULL, NULL }, /* Visual Studio 2012 */
- { "msvcr120", NULL, NULL },
- { "msvcr120d", NULL, NULL }, /* Visual Studio 2013 */
- { "ucrtbase", NULL, NULL },
- { "ucrtbased", NULL, NULL }, /* Visual Studio 2015+ */
- { NULL, 0, NULL }
+ { "msvcrt", NULL, NULL, false },
+ { "msvcrtd", NULL, NULL, false }, /* Visual Studio 6.0 /
mingw */
+ { "msvcr70", NULL, NULL, false },
+ { "msvcr70d", NULL, NULL, false }, /* Visual Studio 2002 */
+ { "msvcr71", NULL, NULL, false },
+ { "msvcr71d", NULL, NULL, false }, /* Visual Studio 2003 */
+ { "msvcr80", NULL, NULL, false },
+ { "msvcr80d", NULL, NULL, false }, /* Visual Studio 2005 */
+ { "msvcr90", NULL, NULL, false },
+ { "msvcr90d", NULL, NULL, false }, /* Visual Studio 2008 */
+ { "msvcr100", NULL, NULL, false },
+ { "msvcr100d", NULL, NULL, false }, /* Visual Studio 2010 */
+ { "msvcr110", NULL, NULL, false },
+ { "msvcr110d", NULL, NULL, false }, /* Visual Studio 2012 */
+ { "msvcr120", NULL, NULL, false },
+ { "msvcr120d", NULL, NULL, false }, /* Visual Studio 2013 */
+ { "ucrtbase", NULL, NULL, false },
+ { "ucrtbased", NULL, NULL, false }, /* Visual Studio 2015+
*/
+ { NULL, 0, NULL, false }
};
int i;
@@ -81,6 +82,16 @@ pgwin32_putenv(const char *envval)
}
else
{
+ if (!rtmodules[i].pinned)
+ {
+ HMODULE tmp;
+ BOOL res = GetModuleHandleEx(
+
GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS
+ |
GET_MODULE_HANDLE_EX_FLAG_PIN,
+
(LPCTSTR)rtmodules[i].hmodule,
+ &tmp);
+ rtmodules[i].pinned = !!res;
+ }
rtmodules[i].putenvFunc = (PUTENVPROC)
GetProcAddress(rtmodules[i].hmodule, "_putenv");
if (rtmodules[i].putenvFunc == NULL)
{
--
2.8.1.windows.1
From fab036a94277cf8add9b3305fff923dd0f1eb97f Mon Sep 17 00:00:00 2001
From: Christian Ullrich <ch...@chrullrich.net>
Date: Tue, 26 Apr 2016 16:00:31 +0200
Subject: [PATCH] Fix the unload race as best we can.
The fix is, unfortunately, to get rid of all caching.
After fixing the load race in the previous commit, a situation was possible *)
where a CRT is loaded, pgwin32_putenv() is called, and then later that CRT is
unloaded again. The next call to pgwin32_putenv() would then attempt to use
the cached function address and crash.
*) It had been possible for untold years, but the probability was very low
because the first call to pgwin32_putenv() occurs early in the process
and used to finalize the list of CRTs the function will admit exist --
whatever is loaded at that point is likely to stay loaded.
---
src/port/win32env.c | 84 ++++++++++++++++++++---------------------------------
1 file changed, 32 insertions(+), 52 deletions(-)
diff --git a/src/port/win32env.c b/src/port/win32env.c
index 20d3665..1e913fc 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -38,67 +38,47 @@ pgwin32_putenv(const char *envval)
*/
#ifdef _MSC_VER
typedef int (_cdecl * PUTENVPROC) (const char *);
- static struct
+ static char *modulenames[] =
{
- char *modulename;
- HMODULE hmodule;
- PUTENVPROC putenvFunc;
- } rtmodules[] =
- {
- { "msvcrt", NULL, NULL },
- { "msvcrtd", NULL, NULL }, /* Visual Studio 6.0 / mingw */
- { "msvcr70", NULL, NULL },
- { "msvcr70d", NULL, NULL }, /* Visual Studio 2002 */
- { "msvcr71", NULL, NULL },
- { "msvcr71d", NULL, NULL }, /* Visual Studio 2003 */
- { "msvcr80", NULL, NULL },
- { "msvcr80d", NULL, NULL }, /* Visual Studio 2005 */
- { "msvcr90", NULL, NULL },
- { "msvcr90d", NULL, NULL }, /* Visual Studio 2008 */
- { "msvcr100", NULL, NULL },
- { "msvcr100d", NULL, NULL }, /* Visual Studio 2010 */
- { "msvcr110", NULL, NULL },
- { "msvcr110d", NULL, NULL }, /* Visual Studio 2012 */
- { "msvcr120", NULL, NULL },
- { "msvcr120d", NULL, NULL }, /* Visual Studio 2013 */
- { "ucrtbase", NULL, NULL },
- { "ucrtbased", NULL, NULL }, /* Visual Studio 2015+ */
- { NULL, 0, NULL }
+ "msvcrt",
+ "msvcrtd", /* Visual Studio 6.0 / mingw */
+ "msvcr70",
+ "msvcr70d", /* Visual Studio 2002 */
+ "msvcr71",
+ "msvcr71d", /* Visual Studio 2003 */
+ "msvcr80",
+ "msvcr80d", /* Visual Studio 2005 */
+ "msvcr90",
+ "msvcr90d", /* Visual Studio 2008 */
+ "msvcr100",
+ "msvcr100d", /* Visual Studio 2010 */
+ "msvcr110",
+ "msvcr110d", /* Visual Studio 2012 */
+ "msvcr120",
+ "msvcr120d", /* Visual Studio 2013 */
+ "ucrtbase",
+ "ucrtbased", /* Visual Studio 2015+ */
+ NULL
};
int i;
- for (i = 0; rtmodules[i].modulename; i++)
+ /*
+ * Call the _putenv() function in all loaded CRTs.
+ * We cannot cache any information about loaded DLLs or
+ * export addresses in them because this can change at
+ * runtime.
+ */
+ for (i = 0; modulenames[i]; i++)
{
- if (rtmodules[i].putenvFunc == NULL)
+ HMODULE hmodule = GetModuleHandle(modulenames[i]);
+ if (hmodule)
{
- if (rtmodules[i].hmodule == NULL)
- {
- /* Try to find this DLL */
- rtmodules[i].hmodule =
GetModuleHandle(rtmodules[i].modulename);
- if (rtmodules[i].hmodule == NULL)
- {
- continue;
- }
- else
- {
- rtmodules[i].putenvFunc = (PUTENVPROC)
GetProcAddress(rtmodules[i].hmodule, "_putenv");
- if (rtmodules[i].putenvFunc == NULL)
- {
- continue;
- }
- }
- }
- else
+ PUTENVPROC putenvFunc = GetProcAddress(hmodule,
"_putenv");
+ if (putenvFunc)
{
- /*
- * Module loaded, but we did not find the
function last time.
- * We're not going to find it this time
either...
- */
- continue;
+ putenvFunc(envval);
}
}
- /* At this point, putenvFunc is set or we have exited the loop
*/
- rtmodules[i].putenvFunc(envval);
}
#endif /* _MSC_VER */
--
2.8.1.windows.1
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
