pgsql: Add putenv support for msvcrt from Visual Studio 2013
Add putenv support for msvcrt from Visual Studio 2013
This was missed when VS 2013 support was added.
Michael Paquier
Branch
------
master
Details
-------
http://git.postgresql.org/pg/commitdiff/9f633b404cb3be6139f8dfdea00538489ffef9ab
Modified Files
--------------
src/port/win32env.c | 3 +++
1 file changed, 3 insertions(+)
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
* Magnus Hagander wrote:
Add putenv support for msvcrt from Visual Studio 2013
This was missed when VS 2013 support was added.
Michael Paquier
Branch
------
masterDetails
-------
http://git.postgresql.org/pg/commitdiff/9f633b404cb3be6139f8dfdea00538489ffef9ab
Just noticed something. This DLL detection by name has never worked in
debug builds where the DLL names end in "d". Is that important?
--
Christian
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
On Sun, Apr 24, 2016 at 9:56 PM, Christian Ullrich <chris@chrullrich.net>
wrote:
* Magnus Hagander wrote:
Add putenv support for msvcrt from Visual Studio 2013
This was missed when VS 2013 support was added.
Michael Paquier
Branch
------
masterDetails
-------http://git.postgresql.org/pg/commitdiff/9f633b404cb3be6139f8dfdea00538489ffef9ab
Just noticed something. This DLL detection by name has never worked in
debug builds where the DLL names end in "d". Is that important?
That's an interesting point. I guess our release builds are never with
debugging info - but could it make the buildfarm "wrong"?
Fixing it should probably be as easy as trying each dll with the specified
name and also with a "d" as a suffix?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
* Magnus Hagander wrote:
On Sun, Apr 24, 2016 at 9:56 PM, Christian Ullrich <chris@chrullrich.net>
wrote:* Magnus Hagander wrote:
Add putenv support for msvcrt from Visual Studio 2013
http://git.postgresql.org/pg/commitdiff/9f633b404cb3be6139f8dfdea00538489ffef9ab
Just noticed something. This DLL detection by name has never worked in
debug builds where the DLL names end in "d". Is that important?
That's an interesting point. I guess our release builds are never with
debugging info - but could it make the buildfarm "wrong"?Fixing it should probably be as easy as trying each dll with the specified
name and also with a "d" as a suffix?
I think so, yes.
Personally, I would have expected that at least the debug/release DLLs
of a single CRT version would somehow share their environment, but I
tried it and they don't.
--
Christian
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
On 04/25/2016 09:27 AM, Christian Ullrich wrote:
* Magnus Hagander wrote:
On Sun, Apr 24, 2016 at 9:56 PM, Christian Ullrich
<chris@chrullrich.net>
wrote:* Magnus Hagander wrote:
Add putenv support for msvcrt from Visual Studio 2013
http://git.postgresql.org/pg/commitdiff/9f633b404cb3be6139f8dfdea00538489ffef9abJust noticed something. This DLL detection by name has never worked in
debug builds where the DLL names end in "d". Is that important?That's an interesting point. I guess our release builds are never with
debugging info - but could it make the buildfarm "wrong"?Fixing it should probably be as easy as trying each dll with the
specified
name and also with a "d" as a suffix?I think so, yes.
Personally, I would have expected that at least the debug/release DLLs
of a single CRT version would somehow share their environment, but I
tried it and they don't.
What if both are present? Is a release build prevented from loading a
debug dll and vice versa?
Alternatively, can we detect at compile time if we are a debug build and
if so add the suffix?
cheers
andrew
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
* Andrew Dunstan wrote:
On 04/25/2016 09:27 AM, Christian Ullrich wrote:
* Magnus Hagander wrote:
On Sun, Apr 24, 2016 at 9:56 PM, Christian Ullrich
<chris@chrullrich.net> wrote:
Just noticed something. This DLL detection by name has never worked in
debug builds where the DLL names end in "d". Is that important?That's an interesting point. I guess our release builds are never with
debugging info - but could it make the buildfarm "wrong"?
What if both are present? Is a release build prevented from loading a
debug dll and vice versa?
Debug and release are simply two separate CRTs. If your process contains
a module that needs the one, and another that needs the other, you will
have both loaded at once.
I had hoped they might share state, but they don't, at least as far as
putenv()/getenv() are concerned.
Alternatively, can we detect at compile time if we are a debug build and
if so add the suffix?
No; same reason as above.
--
Christian
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
* Christian Ullrich wrote:
* Andrew Dunstan wrote:
What if both are present? Is a release build prevented from loading a
debug dll and vice versa?Debug and release are simply two separate CRTs. If your process contains
a module that needs the one, and another that needs the other, you will
have both loaded at once.
pgwin32_putenv() is the gift that keeps giving.
According to its comment, it is not required that all modules exchanging
calls with postgres.exe have to be built with the same CRT version (100,
110, 120, etc.). Is it?
If not, the logic that remembers negative results from GetModuleHandle()
(i.e. gives up forever on each possible CRT once it does not find it) is
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.
If that code is in there because it has some noticeable performance
advantage, the negative results could probably be reset in SQL LOAD,
rather than just not remembering them anymore.
This comment is also incomplete then:
/*
* Module loaded, but we did not find the function last time.
* We're not going to find it this time either...
*/
This else branch is also taken if the module handle is set to
INVALID_HANDLE_VALUE because the module was not found in a previous call.
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.
One situation I can think of where this could occur is if an extension
loaded with LOAD creates a COM in-proc server from a DLL built with yet
another CRT, and when that object is released, either FreeLibrary()
(transitively) or CoFreeUnusedLibraries() (directly) boots that CRT (if
they do; it's possible that a CRT, once loaded, stays loaded.)
Finally: A nonzero handle returned from GetModuleHandle() is not
something that needs to be CloseHandle()d. It is not actually a handle,
but a pointer to the base (load) address of the module, although the
documentation for GetModuleHandle() is careful not to admit that.
The value it is compared against to see whether we have seen the module
before should be NULL, not 0.
It's getting a bit late for me today, but I will do the necessary
experimentation and try to come up with a POC patch to fix whatever of
the above I can actually prove to be real. Should anyone know for sure
that I'm completely off track on something, better yet, everything,
please let me know.
I should finish thinking before posting, then I would not have to reply
to myself so often.
--
Christian
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
* 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
Attachments:
0001-Cleanups-in-pgwin32_putenv.patchtext/plain; charset=UTF-8; name=0001-Cleanups-in-pgwin32_putenv.patchDownload
From d74e778d8abbfea244bacbeb352cadc737032e85 Mon Sep 17 00:00:00 2001
From: Christian Ullrich <chris@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
0002-Fix-the-load-race-in-pgwin32_putenv-and-open-the-unl.patchtext/plain; charset=UTF-8; name=0002-Fix-the-load-race-in-pgwin32_putenv-and-open-the-unl.patchDownload
From e1c705b2657dd5b93c4dd2683b9032a656396571 Mon Sep 17 00:00:00 2001
From: Christian Ullrich <chris@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
0003-Pin-any-DLL-as-soon-as-we-see-it-to-avoid-the-unload.patchtext/plain; charset=UTF-8; name=0003-Pin-any-DLL-as-soon-as-we-see-it-to-avoid-the-unload.patchDownload
From aa129120b12c4ffd907b8765b30044691696a1b2 Mon Sep 17 00:00:00 2001
From: Christian Ullrich <chris@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
0004-Fix-the-unload-race-as-best-we-can.patchtext/plain; charset=UTF-8; name=0004-Fix-the-unload-race-as-best-we-can.patchDownload
From fab036a94277cf8add9b3305fff923dd0f1eb97f Mon Sep 17 00:00:00 2001
From: Christian Ullrich <chris@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
On Wed, Apr 27, 2016 at 2:39 AM, Christian Ullrich <chris@chrullrich.net> wrote:
* 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
\
`- 00040001 is just some various fixes to set the stage.
0002 fixes this "load race" by not remembering a negative result anymore.
However, ...
From 0001, which does not apply anymore on HEAD because of the
integration with MS2015:
if (rtmodules[i].putenvFunc == NULL)
{
- CloseHandle(rtmodules[i].hmodule);
rtmodules[i].hmodule = INVALID_HANDLE_VALUE;
continue;
}
Nice catch. This portion is a bug and should be backpatched. As far as
I can read from MS docs, GetModuleHandle() retrieves an existing
handle so there is no need to free it. Or that would fail.
And actually, by looking at those patches, isn't it a dangerous
practice to be able to load multiple versions of the same DLL routines
in the same workspace? I have personally very bad souvenirs with that,
and particularly loading multiple versions of msvcr into the same
workspace can cause unwanted crashes and corruptions of processes. In
short I mean this thing: https://en.wikipedia.org/wiki/DLL_Hell.
So, shouldn't we first make the DLL list a little bit more severe
depending on the value of _MSC_VER? I would mean something like that:
#ifdef _MSC_VER >= 1900
{"ucrtbase", NULL, NULL},
#elif _MSC_VER >= 1800
{"msvcr120", NULL, NULL},
#elif etc, etc.
[...]
#endif
This would require modules to be built using the same msvc version as
the core server, but that's better than just plain crash if loaded
DLLs corrupt the stack. Am I missing something?
--
Michael
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
* Michael Paquier wrote:
On Wed, Apr 27, 2016 at 2:39 AM, Christian Ullrich
<chris@chrullrich.net> wrote:
* Christian Ullrich wrote:
And actually, by looking at those patches, isn't it a dangerous
practice to be able to load multiple versions of the same DLL routines
in the same workspace? I have personally very bad souvenirs with that,
No, it is exactly what the version-specific CRTs are meant to allow.
Each module uses the CRT version it needs, and they don't share any
state, so absent bugs, they cannot conflict.
Of the processes currently running on my system, 25 have more than one
CRT loaded (one has three, the others two).
and particularly loading multiple versions of msvcr into the same
workspace can cause unwanted crashes and corruptions of processes. In
short I mean this thing: https://en.wikipedia.org/wiki/DLL_Hell.
That was about DLLs existing in different versions with the same file
name, and installers replacing them with their own, leading to problems
with other applications that expected to load their preferred version.
This does not apply to the multiple-CRT situation, because all minor
versions of a given CRT are supposed to be ABI compatible.
So, shouldn't we first make the DLL list a little bit more severe
depending on the value of _MSC_VER? I would mean something like that:
#ifdef _MSC_VER >= 1900
{"ucrtbase", NULL, NULL},
#elif _MSC_VER >= 1800
{"msvcr120", NULL, NULL},
#elif etc, etc.
[...]
#endifThis would require modules to be built using the same msvc version as
the core server, but that's better than just plain crash if loaded
DLLs corrupt the stack. Am I missing something?
Yes: This turns (this part of) pgwin32_putenv() into a great big NOP. We
call putenv() anyway on the very last line of the function, so if we
require common-CRT builds, that call alone (together with the
SetEnvironmentVariable() just above) is sufficient.
That said, introducing this requirement would be a very significant
change. I'm not sure how many independently maintained compiled
extensions there are, but this would mean that their developers would
have to have the matching VS versions for every PG distribution they
want to support. Even if that's just EDB, it still is a lot of effort.
My conclusion from April stands:
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?
--
Christian
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
On Thu, Sep 1, 2016 at 4:03 PM, Christian Ullrich <chris@chrullrich.net> wrote:
* Michael Paquier wrote:
On Wed, Apr 27, 2016 at 2:39 AM, Christian Ullrich <chris@chrullrich.net>
wrote:* Christian Ullrich wrote:
And actually, by looking at those patches, isn't it a dangerous
practice to be able to load multiple versions of the same DLL routines
in the same workspace? I have personally very bad souvenirs with that,No, it is exactly what the version-specific CRTs are meant to allow. Each
module uses the CRT version it needs, and they don't share any state, so
absent bugs, they cannot conflict.
Hm. OK.
That said, introducing this requirement would be a very significant change.
I'm not sure how many independently maintained compiled extensions there
are, but this would mean that their developers would have to have the
matching VS versions for every PG distribution they want to support. Even if
that's just EDB, it still is a lot of effort.
Yes. FWIW in my stuff everything gets compiled based on the same VS
version and bundled in the same msi, including a set of extensions
compiled from source, but perhaps my sight is too narrow in this
area... Well let's forget about that.
My conclusion from April stands:
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?
Looking at the commit logs and 741e4ad7 that does not sound like a good idea.
+ 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;
+ }
It is better to avoid !!. See for example 1a7a436 that avoided
problems with VS2015 as far as I recall.In order to avoid any problems with the load and unload windows, my
bet goes for 0001, 0002 and 0003, with the last two patches merged
together, 0001 being only a set of independent fixes. That's ugly, but
that looks the safest course of actions. I have rebased/rewritten the
patches as attached. Thoughts?
--
Michael
Attachments:
0001-Cleanups-in-pgwin32_putenv.patchtext/x-diff; charset=US-ASCII; name=0001-Cleanups-in-pgwin32_putenv.patchDownload
From b809a0b1c323529c2d7460962a3c688ad8aec3f4 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 6 Sep 2016 15:51:55 +0900
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 | 61 +++++++++++++++++++++++++----------------------------
1 file changed, 29 insertions(+), 32 deletions(-)
diff --git a/src/port/win32env.c b/src/port/win32env.c
index e64065c..77f8334 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -45,36 +45,34 @@ 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 */
- {
- "ucrtbase", 0, NULL
- }, /* Visual Studio 2015 and later */
- {
- NULL, 0, NULL
- }
+ /* Visual Studio 6.0 / mingw */
+ {"msvcrt", NULL, NULL},
+ {"msvcrtd", NULL, NULL},
+ /* Visual Studio 2002 */
+ {"msvcr70", NULL, NULL},
+ {"msvcr70d", NULL, NULL},
+ /* Visual Studio 2003 */
+ {"msvcr71", NULL, NULL},
+ {"msvcr71d", NULL, NULL},
+ /* Visual Studio 2005 */
+ {"msvcr80", NULL, NULL},
+ {"msvcr80d", NULL, NULL},
+ /* Visual Studio 2008 */
+ {"msvcr90", NULL, NULL},
+ {"msvcr90d", NULL, NULL},
+ /* Visual Studio 2010 */
+ {"msvcr100", NULL, NULL},
+ {"msvcr100d", NULL, NULL},
+ /* Visual Studio 2012 */
+ {"msvcr110", NULL, NULL},
+ {"msvcr110d", NULL, NULL},
+ /* Visual Studio 2013 */
+ {"msvcr120", NULL, NULL},
+ {"msvcr120d", NULL, NULL},
+ /* Visual Studio 2015 and later */
+ {"ucrtbase", NULL, NULL},
+ {"ucrtbased", NULL, NULL},
+ {NULL, NULL, NULL}
};
int i;
@@ -82,7 +80,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);
@@ -100,7 +98,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.10.0
0002-Fix-the-load-race-in-pgwin32_putenv-and-open-the-unl.patchtext/x-diff; charset=US-ASCII; name=0002-Fix-the-load-race-in-pgwin32_putenv-and-open-the-unl.patchDownload
From 27d47434e83d5d81b912a194624910c655729431 Mon Sep 17 00:00:00 2001
From: Christian Ullrich <chris@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 77f8334..3f56ba8 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -82,15 +82,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
@@ -98,7 +93,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.10.0
0003-Pin-any-DLL-as-soon-as-seen-when-looking-for-_putenv.patchtext/x-diff; charset=US-ASCII; name=0003-Pin-any-DLL-as-soon-as-seen-when-looking-for-_putenv.patchDownload
From 49ca06e0042caf5c5264aed91f8f38f062dacb60 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 6 Sep 2016 16:02:00 +0900
Subject: [PATCH 3/3] Pin any DLL as soon as seen when looking for _putenv on
Windows
This prevents the DLL to be unloaded, and maintains the load until the
process is terminated.
---
src/port/win32env.c | 54 ++++++++++++++++++++++++++++++++++-------------------
1 file changed, 35 insertions(+), 19 deletions(-)
diff --git a/src/port/win32env.c b/src/port/win32env.c
index 3f56ba8..ed20f1c 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -43,36 +43,37 @@ pgwin32_putenv(const char *envval)
char *modulename;
HMODULE hmodule;
PUTENVPROC putenvFunc;
+ bool pinned;
} rtmodules[] =
{
/* Visual Studio 6.0 / mingw */
- {"msvcrt", NULL, NULL},
- {"msvcrtd", NULL, NULL},
+ {"msvcrt", NULL, NULL, false},
+ {"msvcrtd", NULL, NULL, false},
/* Visual Studio 2002 */
- {"msvcr70", NULL, NULL},
- {"msvcr70d", NULL, NULL},
+ {"msvcr70", NULL, NULL, false},
+ {"msvcr70d", NULL, NULL, false},
/* Visual Studio 2003 */
- {"msvcr71", NULL, NULL},
- {"msvcr71d", NULL, NULL},
+ {"msvcr71", NULL, NULL, false},
+ {"msvcr71d", NULL, NULL, false},
/* Visual Studio 2005 */
- {"msvcr80", NULL, NULL},
- {"msvcr80d", NULL, NULL},
+ {"msvcr80", NULL, NULL, false},
+ {"msvcr80d", NULL, NULL, false},
/* Visual Studio 2008 */
- {"msvcr90", NULL, NULL},
- {"msvcr90d", NULL, NULL},
+ {"msvcr90", NULL, NULL, false},
+ {"msvcr90d", NULL, NULL, false},
/* Visual Studio 2010 */
- {"msvcr100", NULL, NULL},
- {"msvcr100d", NULL, NULL},
+ {"msvcr100", NULL, NULL, false},
+ {"msvcr100d", NULL, NULL, false},
/* Visual Studio 2012 */
- {"msvcr110", NULL, NULL},
- {"msvcr110d", NULL, NULL},
+ {"msvcr110", NULL, NULL, false},
+ {"msvcr110d", NULL, NULL, false},
/* Visual Studio 2013 */
- {"msvcr120", NULL, NULL},
- {"msvcr120d", NULL, NULL},
+ {"msvcr120", NULL, NULL, false},
+ {"msvcr120d", NULL, NULL, false},
/* Visual Studio 2015 and later */
- {"ucrtbase", NULL, NULL},
- {"ucrtbased", NULL, NULL},
- {NULL, NULL, NULL}
+ {"ucrtbase", NULL, NULL, false},
+ {"ucrtbased", NULL, NULL, false},
+ {NULL, NULL, NULL, false}
};
int i;
@@ -90,6 +91,21 @@ pgwin32_putenv(const char *envval)
}
else
{
+ /*
+ * Pin this DLL handle as soon as possible to avoid it
+ * to be unloaded until the process terminates.
+ */
+ 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 != NULL);
+ }
+
rtmodules[i].putenvFunc = (PUTENVPROC) GetProcAddress(rtmodules[i].hmodule, "_putenv");
if (rtmodules[i].putenvFunc == NULL)
{
--
2.10.0
* Michael Paquier wrote:
On Thu, Sep 1, 2016 at 4:03 PM, Christian Ullrich <chris@chrullrich.net> wrote:
My conclusion from April stands:
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?Looking at the commit logs and 741e4ad7 that does not sound like a good idea.
Well, I still maintain that if it doesn't work and has never worked,
getting rid of it is better than making it work six years after the
fact. OTOH, there may have been cases where it did actually work,
perhaps those gnuwin32 libraries that were mentioned in the comment
before it was changed in that commit above, if they were loaded before
the first call to the function.
OTTH, wouldn't it be funny if fixing it actually broke something that
worked accidentally because it *didn't* get the updated environment? I
think that is at least as likely as suddenly getting excited reports
that something now works that hasn't before.
It is better to avoid !!. See for example 1a7a436 that avoided
problems with VS2015 as far as I recall.
Agreed, thanks for noticing. This change creates a warning, however,
because GetModuleHandleEx() returns BOOL, not HMODULE. Updated 0003
attached, simplified over my original one.
In order to avoid any problems with the load and unload windows, my
bet goes for 0001, 0002 and 0003, with the last two patches merged
together, 0001 being only a set of independent fixes. That's ugly, but
that looks the safest course of actions. I have rebased/rewritten the
patches as attached. Thoughts?
In lieu of convincing you to drop the entire thing, yes, that looks
about right, except for the BOOL thing.
--
Christian
Attachments:
0003-Pin-any-DLL-as-soon-as-seen-when-looking-for-_putenv.patchtext/plain; charset=UTF-8; name=0003-Pin-any-DLL-as-soon-as-seen-when-looking-for-_putenv.patchDownload
From dfbe7384b309c20d7733b0d18e6819302a483d95 Mon Sep 17 00:00:00 2001
From: Christian Ullrich <chris@chrullrich.net>
Date: Tue, 6 Sep 2016 10:02:06 +0200
Subject: [PATCH] Pin any DLL as soon as seen when looking for _putenv()
---
src/port/win32env.c | 54 ++++++++++++++++++++++++++++++++++-------------------
1 file changed, 35 insertions(+), 19 deletions(-)
diff --git a/src/port/win32env.c b/src/port/win32env.c
index 3f56ba8..7417b60 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -43,36 +43,37 @@ pgwin32_putenv(const char *envval)
char *modulename;
HMODULE hmodule;
PUTENVPROC putenvFunc;
+ BOOL pinned;
} rtmodules[] =
{
/* Visual Studio 6.0 / mingw */
- {"msvcrt", NULL, NULL},
- {"msvcrtd", NULL, NULL},
+ {"msvcrt", NULL, NULL, FALSE},
+ {"msvcrtd", NULL, NULL, FALSE},
/* Visual Studio 2002 */
- {"msvcr70", NULL, NULL},
- {"msvcr70d", NULL, NULL},
+ {"msvcr70", NULL, NULL, FALSE},
+ {"msvcr70d", NULL, NULL, FALSE},
/* Visual Studio 2003 */
- {"msvcr71", NULL, NULL},
- {"msvcr71d", NULL, NULL},
+ {"msvcr71", NULL, NULL, FALSE},
+ {"msvcr71d", NULL, NULL, FALSE},
/* Visual Studio 2005 */
- {"msvcr80", NULL, NULL},
- {"msvcr80d", NULL, NULL},
+ {"msvcr80", NULL, NULL, FALSE},
+ {"msvcr80d", NULL, NULL, FALSE},
/* Visual Studio 2008 */
- {"msvcr90", NULL, NULL},
- {"msvcr90d", NULL, NULL},
+ {"msvcr90", NULL, NULL, FALSE},
+ {"msvcr90d", NULL, NULL, FALSE},
/* Visual Studio 2010 */
- {"msvcr100", NULL, NULL},
- {"msvcr100d", NULL, NULL},
+ {"msvcr100", NULL, NULL, FALSE},
+ {"msvcr100d", NULL, NULL, FALSE},
/* Visual Studio 2012 */
- {"msvcr110", NULL, NULL},
- {"msvcr110d", NULL, NULL},
+ {"msvcr110", NULL, NULL, FALSE},
+ {"msvcr110d", NULL, NULL, FALSE},
/* Visual Studio 2013 */
- {"msvcr120", NULL, NULL},
- {"msvcr120d", NULL, NULL},
+ {"msvcr120", NULL, NULL, FALSE},
+ {"msvcr120d", NULL, NULL, FALSE},
/* Visual Studio 2015 and later */
- {"ucrtbase", NULL, NULL},
- {"ucrtbased", NULL, NULL},
- {NULL, NULL, NULL}
+ {"ucrtbase", NULL, NULL, FALSE},
+ {"ucrtbased", NULL, NULL, FALSE},
+ {NULL, NULL, NULL, FALSE}
};
int i;
@@ -90,6 +91,21 @@ pgwin32_putenv(const char *envval)
}
else
{
+ /*
+ * Pin this DLL handle as soon as possible to avoid it
+ * to be unloaded until the process terminates.
+ */
+ 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.10.0.windows.1
On 6 Sep. 2016 15:12, "Michael Paquier" <michael.paquier@gmail.com> wrote:
On Thu, Sep 1, 2016 at 4:03 PM, Christian Ullrich <chris@chrullrich.net>
wrote:
That said, introducing this requirement would be a very significant
change.
I'm not sure how many independently maintained compiled extensions there
are, but this would mean that their developers would have to have the
matching VS versions for every PG distribution they want to support.
Even if
that's just EDB, it still is a lot of effort.
Yes. FWIW in my stuff everything gets compiled based on the same VS
version and bundled in the same msi, including a set of extensions
compiled from source, but perhaps my sight is too narrow in this
area... Well let's forget about that.
3rd party extensions may not and may not be able to. Most obvious example
is people building things with mingw.
This is just expected to work on win32. Breaking this assumption will cause
pain. Requiring a single unified C runtime across the process isn't viable.
It isn't like Unix. You might have a legacy DLL compiled with Borland C
that you're wrapping up to expose as an extension using mingw to link into
a Pg compiled with MSVC.
On Tue, Sep 6, 2016 at 5:36 PM, Christian Ullrich <chris@chrullrich.net> wrote:
* Michael Paquier wrote:
In order to avoid any problems with the load and unload windows, my
bet goes for 0001, 0002 and 0003, with the last two patches merged
together, 0001 being only a set of independent fixes. That's ugly, but
that looks the safest course of actions. I have rebased/rewritten the
patches as attached. Thoughts?In lieu of convincing you to drop the entire thing, yes, that looks about
right, except for the BOOL thing.
Yes, right. Thanks. Patch 0001 is definitely something that should be
applied and backpatched, the CloseHandle() call is buggy. Now 0002 and
0003 are improving things, but there have been no reports regarding
problems in this area, so this could just be applied to master I
guess. Christian, does that sound right?
By the way, how is it decided that a DLL gets unloaded in a process if
it is not pinned? Is that something the OS decides?
--
Michael
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
* Michael Paquier wrote:
On Tue, Sep 6, 2016 at 5:36 PM, Christian Ullrich <chris@chrullrich.net> wrote:
* Michael Paquier wrote:
In order to avoid any problems with the load and unload windows, my
bet goes for 0001, 0002 and 0003, with the last two patches merged
together, 0001 being only a set of independent fixes. That's ugly, but
that looks the safest course of actions. I have rebased/rewritten the
patches as attached. Thoughts?In lieu of convincing you to drop the entire thing, yes, that looks about
right, except for the BOOL thing.Yes, right. Thanks. Patch 0001 is definitely something that should be
applied and backpatched, the CloseHandle() call is buggy. Now 0002 and
0003 are improving things, but there have been no reports regarding
problems in this area, so this could just be applied to master I
guess. Christian, does that sound right?
Yes.
By the way, how is it decided that a DLL gets unloaded in a process if
it is not pinned? Is that something the OS decides?
Reference counting in LoadLibrary() and FreeLibrary(), among other
places. For example, GetModuleHandleEx() (but not the non-Ex) will by
default increment the counter.
--
Christian
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
(Just remembered to remove pgsql-committers here).
On Tue, Sep 6, 2016 at 9:21 PM, Christian Ullrich <chris@chrullrich.net> wrote:
* Michael Paquier wrote:
On Tue, Sep 6, 2016 at 5:36 PM, Christian Ullrich <chris@chrullrich.net>
wrote:* Michael Paquier wrote:
In order to avoid any problems with the load and unload windows, my
bet goes for 0001, 0002 and 0003, with the last two patches merged
together, 0001 being only a set of independent fixes. That's ugly, but
that looks the safest course of actions. I have rebased/rewritten the
patches as attached. Thoughts?In lieu of convincing you to drop the entire thing, yes, that looks about
right, except for the BOOL thing.Yes, right. Thanks. Patch 0001 is definitely something that should be
applied and backpatched, the CloseHandle() call is buggy. Now 0002 and
0003 are improving things, but there have been no reports regarding
problems in this area, so this could just be applied to master I
guess. Christian, does that sound right?Yes.
OK, let's get to the next step of the game and get a committer to look
at this patch.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 6, 2016 at 9:36 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
OK, let's get to the next step of the game and get a committer to look
at this patch.
Moved to next CF. It would be good to get a committer on this one. We
have come on a conclusion on what to do. Actually, 0001 can be just
HEAD-only per the lack of complaints.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Apr 26, 2016 at 07:39:29PM +0200, Christian Ullrich wrote:
* 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
\
`- 00040001 is just some various fixes to set the stage.
Patch 1 looks good, except that it should be three patches:
- cosmetic parts: change whitespace and s/0/NULL/
- remove CloseHandle() call
- probe for debug CRT modules, not just release CRT modules
Please split along those boundaries. I plan to back-patch all of that. I've
seen some gettext builds mysteriously ignore "SET lc_messages = ..."; ignoring
debug CRTs could have been the cause.
I tested this with a project
(<https://bitbucket.org/chrullrich/pgputenv-demo>) that contains two DLLs:
That's a pithy test; thanks for assembling it.
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.
I think you can fix it by abandoning GetModuleHandle() in favor of
GetModuleHandleEx() + GetProcessAddress() + FreeLibrary(). I recommend also
moving the SetEnvironmentVariable() call before the putenv calls. That way,
if a CRT loads while pgwin32_putenv() is executing, the newly-loaded CRT will
always have the latest value. (I'm assuming CRTs populate their environment
from GetEnvironmentStrings(), because I can't think of an alternative.)
As a separate patch, I am inclined to remove the "#ifdef _MSC_VER" directive,
activating its enclosed code under all compilers. A MinGW-built postgres.exe
has the same need to update all CRTs.
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?
pgwin32_putenv() originated, in commit 0154345, to make "SET lc_messages =
..." effective when gettext uses a different CRT from postgres.exe. I suspect
it also makes krb_server_keyfile effective when GSS uses a different CRT.
Those are achievements worth keeping. I'm not surprised about the lack of
complaints, because environment variables don't often change after backend
startup. Here are some ways one could notice the difference between master
and patches 2+3 or 2+4:
- Use shared_preload_libraries to load a module that reads LC_CTYPE or
LC_COLLATE. CheckMyDatabase() sets those variables subsequent to
process_shared_preload_libraries().
- Load, at any time, a module that reads LC_MESSAGES. There's little reason
to read that variable outside of gettext. A module could use a gettext DLL
other than the postgres.exe gettext DLL, but such a module would need to
work around pg_bindtextdomain() always using the postgres.exe gettext.
- Load, at any time, a module that itself changes environment variables, other
than LC_MESSAGES, after backend startup. I suspect PL/Python suffices.
Those are plausible scenarios, but they're sufficiently specialized that
problems could lie unnoticed or undiagnosed for years. I lean against
back-patching anything from patches 2, 3 or 4.
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.
I prefer the simplicity of abandoning the cache (patch 4), if it performs
decently. Would you compare the performance of patch 1, patches 1+2+3, and
patches 1+2+4? This should measure the right thing (after substituting
@libdir@ for your environment):
CREATE FUNCTION putenv(text)
RETURNS void
AS '@libdir@/regress.dll', 'regress_putenv'
LANGUAGE C STRICT;
\timing on
SELECT count(putenv('foo=' || n)) FROM generate_series(1,1000000) t(n);
(I'm interested for the sake of backend startup time. I recall nine putenv()
in every backend startup, seven in main() and two in CheckMyDatabase().)
Thanks,
nm
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Noah Misch wrote:
On Tue, Apr 26, 2016 at 07:39:29PM +0200, Christian Ullrich
wrote:
* Christian Ullrich wrote:
Patch 1 looks good, except that it should be three patches:
- cosmetic parts: change whitespace and s/0/NULL/
- remove CloseHandle() call
- probe for debug CRT modules, not just release CRT modules
Attached as 0001, 0002, 0003, in that order.
0004 is what used to be 0002, it disables caching of "DLL not
loaded" results.
I recommend also moving the SetEnvironmentVariable() call before
the putenv calls. That way, if a CRT loads while pgwin32_putenv()
is executing, the newly-loaded CRT will always have the latest
value.
Agreed, attached as 0005.
0006 was previously known as 0004, removing all caching.
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.I think you can fix it by abandoning GetModuleHandle() in favor
of GetModuleHandleEx() + GetProcessAddress() + FreeLibrary().
Attached as 0007.
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.
This is now 0008.
Patch topology: master --- 1 .. 5 --- 6 --- 7
\
`- 8
I prefer the simplicity of abandoning the cache (patch 4), if it
performs decently. Would you compare the performance of patch 1,
patches 1+2+3, and patches 1+2+4? This should measure the right
thing (after substituting @libdir@ for your environment):
Tested with release builds; Core i7-6700K (quad/HT; 4000 MHz).
I did three runs each, and they were always within 0.5 % of each
other's run time.
- patch 1 (now 1-3): 24 μs/iteration (24 s for 1e6)
- patch 1+2+3 (now 1-5 + 8): 29 μs/iteration
- patch 1+2+4 (now 1-7): 30 μs/iteration
I also did a debug build with 1+2+4 that came in at 84 μs/iteration.
--
Christian
... now how do I get all the dangling debris out of the repo ...
Attachments:
0008-getmodulehandleex-pin.patchapplication/octet-stream; name=0008-getmodulehandleex-pin.patchDownload
From f7013462ccc10781bf1ac722d6f0f8c5b7c31b7b Mon Sep 17 00:00:00 2001
From: Christian Ullrich <chris@chrullrich.net>
Date: Tue, 6 Sep 2016 10:02:06 +0200
Subject: [PATCH] Pin any DLL as soon as seen when looking for _putenv().
---
src/port/win32env.c | 56 ++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 38 insertions(+), 18 deletions(-)
diff --git a/src/port/win32env.c b/src/port/win32env.c
index d6391b0..48c4d82 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -75,26 +75,27 @@ 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 and later */
+ { "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 and later */
{ NULL, NULL, NULL }
};
int i;
@@ -113,6 +114,25 @@ pgwin32_putenv(const char *envval)
}
else
{
+ /*
+ * Pin this DLL handle as soon as possible to avoid it
+ * to be unloaded until the process terminates.
+ */
+ if (!rtmodules[i].pinned)
+ {
+ /*
+ * This leaks the new reference, but that does
+ * not matter because we pin the DLL anyway.
+ */
+ 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 != 0;
+ }
+
rtmodules[i].putenvFunc = (PUTENVPROC) GetProcAddress(rtmodules[i].hmodule, "_putenv");
if (rtmodules[i].putenvFunc == NULL)
{
--
2.10.2.windows.1
0001-whitespace.patchapplication/octet-stream; name=0001-whitespace.patchDownload
From fefe3a4f571b966e5cd84967c135acbe82fe8d37 Mon Sep 17 00:00:00 2001
From: Christian Ullrich <chris@chrullrich.net>
Date: Wed, 16 Nov 2016 16:07:01 +0100
Subject: [PATCH] Cosmetic fixes (whitespace, NULL instead of 0).
---
src/port/win32env.c | 42 +++++++++++-------------------------------
1 file changed, 11 insertions(+), 31 deletions(-)
diff --git a/src/port/win32env.c b/src/port/win32env.c
index e64065c..478ec1d 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -45,36 +45,16 @@ 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 */
- {
- "ucrtbase", 0, NULL
- }, /* Visual Studio 2015 and later */
- {
- NULL, 0, NULL
- }
+ { "msvcrt", NULL, NULL }, /* Visual Studio 6.0 / mingw */
+ { "msvcr70", NULL, NULL }, /* Visual Studio 2002 */
+ { "msvcr71", NULL, NULL }, /* Visual Studio 2003 */
+ { "msvcr80", NULL, NULL }, /* Visual Studio 2005 */
+ { "msvcr90", NULL, NULL }, /* Visual Studio 2008 */
+ { "msvcr100", NULL, NULL }, /* Visual Studio 2010 */
+ { "msvcr110", NULL, NULL }, /* Visual Studio 2012 */
+ { "msvcr120", NULL, NULL }, /* Visual Studio 2013 */
+ { "ucrtbase", NULL, NULL }, /* Visual Studio 2015 and later */
+ { NULL, NULL, NULL }
};
int i;
@@ -82,7 +62,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);
--
2.10.2.windows.1
0002-closehandle.patchapplication/octet-stream; name=0002-closehandle.patchDownload
From fb9bb4c57c99a2446397b04622164804bd29d4cc Mon Sep 17 00:00:00 2001
From: Christian Ullrich <chris@chrullrich.net>
Date: Wed, 16 Nov 2016 16:11:27 +0100
Subject: [PATCH] Remove unnecessary CloseHandle() call.
An HMODULE from GetModuleHandle() does not need closing.
---
src/port/win32env.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/port/win32env.c b/src/port/win32env.c
index 478ec1d..5aaf100 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -80,7 +80,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.10.2.windows.1
0003-debug-crt.patchapplication/octet-stream; name=0003-debug-crt.patchDownload
From 6e4dff144c6f3d82941ef6a7b31cbea6ca82180e Mon Sep 17 00:00:00 2001
From: Christian Ullrich <chris@chrullrich.net>
Date: Wed, 16 Nov 2016 16:14:04 +0100
Subject: [PATCH] Add support for updating the environment in debug CRTs.
---
src/port/win32env.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/src/port/win32env.c b/src/port/win32env.c
index 5aaf100..2047b03 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -45,15 +45,24 @@ pgwin32_putenv(const char *envval)
PUTENVPROC putenvFunc;
} rtmodules[] =
{
- { "msvcrt", NULL, NULL }, /* Visual Studio 6.0 / mingw */
- { "msvcr70", NULL, NULL }, /* Visual Studio 2002 */
- { "msvcr71", NULL, NULL }, /* Visual Studio 2003 */
- { "msvcr80", NULL, NULL }, /* Visual Studio 2005 */
- { "msvcr90", NULL, NULL }, /* Visual Studio 2008 */
- { "msvcr100", NULL, NULL }, /* Visual Studio 2010 */
- { "msvcr110", NULL, NULL }, /* Visual Studio 2012 */
- { "msvcr120", NULL, NULL }, /* Visual Studio 2013 */
- { "ucrtbase", NULL, NULL }, /* Visual Studio 2015 and later */
+ { "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 and later */
{ NULL, NULL, NULL }
};
int i;
--
2.10.2.windows.1
0004 no-caching-notfound.patchapplication/octet-stream; name="0004 no-caching-notfound.patch"Download
From 18aa44d3b6d2ce2612a6db742e698e76c0dda6c3 Mon Sep 17 00:00:00 2001
From: Christian Ullrich <chris@chrullrich.net>
Date: Wed, 16 Nov 2016 16:16:58 +0100
Subject: [PATCH] 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 2047b03..7bccee6 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.10.2.windows.1
0005-reorder-update.patchapplication/octet-stream; name=0005-reorder-update.patchDownload
From ffb37978ba120522eccf2c0849e46edd94afde67 Mon Sep 17 00:00:00 2001
From: Christian Ullrich <chris@chrullrich.net>
Date: Wed, 16 Nov 2016 16:47:21 +0100
Subject: [PATCH] Do the process environment update first.
This closes a potential race that can happen if a new CRT is loaded while
pgwin32_putenv() is running. Per Noah Misch.
---
src/port/win32env.c | 64 ++++++++++++++++++++++++++---------------------------
1 file changed, 32 insertions(+), 32 deletions(-)
diff --git a/src/port/win32env.c b/src/port/win32env.c
index 7bccee6..d6391b0 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -24,6 +24,38 @@ pgwin32_putenv(const char *envval)
char *cp;
/*
+ * Update the process environment - to make modifications visible to child
+ * processes.
+ *
+ * Need a copy of the string so we can modify it.
+ */
+ envcpy = strdup(envval);
+ if (!envcpy)
+ return -1;
+ cp = strchr(envcpy, '=');
+ if (cp == NULL)
+ {
+ free(envcpy);
+ return -1;
+ }
+ *cp = '\0';
+ cp++;
+ if (strlen(cp))
+ {
+ /*
+ * Only call SetEnvironmentVariable() when we are adding a variable,
+ * not when removing it. Calling it on both crashes on at least
+ * certain versions of MingW.
+ */
+ if (!SetEnvironmentVariable(envcpy, cp))
+ {
+ free(envcpy);
+ return -1;
+ }
+ }
+ free(envcpy);
+
+ /*
* Each version of MSVCRT has its own _putenv() call in the runtime
* library.
*
@@ -102,38 +134,6 @@ pgwin32_putenv(const char *envval)
}
#endif /* _MSC_VER */
- /*
- * Update the process environment - to make modifications visible to child
- * processes.
- *
- * Need a copy of the string so we can modify it.
- */
- envcpy = strdup(envval);
- if (!envcpy)
- return -1;
- cp = strchr(envcpy, '=');
- if (cp == NULL)
- {
- free(envcpy);
- return -1;
- }
- *cp = '\0';
- cp++;
- if (strlen(cp))
- {
- /*
- * Only call SetEnvironmentVariable() when we are adding a variable,
- * not when removing it. Calling it on both crashes on at least
- * certain versions of MingW.
- */
- if (!SetEnvironmentVariable(envcpy, cp))
- {
- free(envcpy);
- return -1;
- }
- }
- free(envcpy);
-
/* Finally, update our "own" cache */
return _putenv(envval);
}
--
2.10.2.windows.1
0006-no-caching-at-all.patchapplication/octet-stream; name=0006-no-caching-at-all.patchDownload
From 84d69b98f9ae5cfe704cef3af44ea42d0a549d73 Mon Sep 17 00:00:00 2001
From: Christian Ullrich <chris@chrullrich.net>
Date: Wed, 16 Nov 2016 16:55:45 +0100
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 d6391b0..25eed06 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -70,67 +70,47 @@ pgwin32_putenv(const char *envval)
*/
#ifdef _MSC_VER
typedef int (_cdecl * PUTENVPROC) (const char *);
- static struct
+ static const 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 and later */
- { NULL, NULL, 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 and later */
+ 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 = (PUTENVPROC) 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.10.2.windows.1
0007-getmodulehandleex-correctness.patchapplication/octet-stream; name=0007-getmodulehandleex-correctness.patchDownload
From 6aafcece307197d7f453b081093b35c5ba00c821 Mon Sep 17 00:00:00 2001
From: Christian Ullrich <chris@chrullrich.net>
Date: Wed, 16 Nov 2016 17:02:27 +0100
Subject: [PATCH] Avoid a possible crash.
Although PostgreSQL itself does not use multithreading, there are always a
number of threads in a process on Windows. If one of them causes a CRT to
unload at the wrong time, we can crash in the _putenv() call. Fix by
keeping our own reference to the loaded DLL as long as we need.
Per suggestion from Noah Misch.
---
src/port/win32env.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/port/win32env.c b/src/port/win32env.c
index 25eed06..5b594e3 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -102,14 +102,20 @@ pgwin32_putenv(const char *envval)
*/
for (i = 0; modulenames[i]; i++)
{
- HMODULE hmodule = GetModuleHandle(modulenames[i]);
- if (hmodule)
+ /*
+ * Make sure nothing else unloads the DLL while we are using it
+ * by creating our own reference to it.
+ */
+ HMODULE hmodule = NULL;
+ BOOL res = GetModuleHandleEx(0, modulenames[i], &hmodule);
+ if (res != 0 && hmodule != NULL)
{
PUTENVPROC putenvFunc = (PUTENVPROC) GetProcAddress(hmodule, "_putenv");
if (putenvFunc)
{
putenvFunc(envval);
}
+ FreeLibrary(hmodule);
}
}
#endif /* _MSC_VER */
--
2.10.2.windows.1
On Wed, Nov 16, 2016 at 12:45 PM, Christian Ullrich
<chris@chrullrich.net> wrote:
I also did a debug build with 1+2+4 that came in at 84 μs/iteration.
Moved to next CF. Christian, perhaps this patch should have an extra
round of reviews?
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Michael Paquier wrote:
On Wed, Nov 16, 2016 at 12:45 PM, Christian Ullrich
<chris@chrullrich.net> wrote:I also did a debug build with 1+2+4 that came in at 84 μs/iteration.
Moved to next CF. Christian, perhaps this patch should have an extra
round of reviews?
It is significantly different from the last version, so yes, of course.
--
Christian
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Nov 29, 2016 at 08:45:13PM +0100, Christian Ullrich wrote:
* Michael Paquier wrote:
On Wed, Nov 16, 2016 at 12:45 PM, Christian Ullrich
<chris@chrullrich.net> wrote:I also did a debug build with 1+2+4 that came in at 84 μs/iteration.
Moved to next CF. Christian, perhaps this patch should have an extra
round of reviews?It is significantly different from the last version, so yes, of course.
Patches 0001 (Cosmetic fixes), 0002 (Remove unnecessary CloseHandle)
and 0003 (support for debug CRTs) look in good shape to me. 0004 (Fix
load race) is 0002 from the previous set, and this change makes sense
in itself.
With 0005 I am seeing a compilation failure: you need to have the
declarations in the _MSC_VER block at the beginning of the routine. It
would be nice to mention in a code comment that this what Noah has
mentioned upthread: if a CRT loads while pgwin32_putenv() is
executing, the newly-loaded CRT will always have the latest value.
Based on that 0006 will need a rebase, nothing huge though.
Removing the caching per the measurements upthread is causing a 1us
regression compared to the full set. Let's do things simple then! This
smells like noise.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* From: Michael Paquier
With 0005 I am seeing a compilation failure: you need to have the
declarations in the _MSC_VER block at the beginning of the routine. It
Sorry, too used to C++.
would be nice to mention in a code comment that this what Noah has
mentioned upthread: if a CRT loads while pgwin32_putenv() is
executing, the newly-loaded CRT will always have the latest value.
I fixed the existing comment by removing the last sentence that is in the wrong place now, but I don't see the point in suddenly starting to explain why it is done this way and not the other.
Based on that 0006 will need a rebase, nothing huge though.
Done, new revisions attached.
--
Christian
Attachments:
0005-Do-the-process-environment-update-first.patchapplication/octet-stream; name=0005-Do-the-process-environment-update-first.patchDownload
From 21019e77e85a65b8f362426f269a748ff21baa43 Mon Sep 17 00:00:00 2001
From: Christian Ullrich <christian.ullrich@traditionsa.lu>
Date: Wed, 30 Nov 2016 16:19:55 +0100
Subject: [PATCH 5/7] Do the process environment update first.
This closes a potential race that can happen if a new CRT is loaded while
pgwin32_putenv() is running. Per Noah Misch.
---
src/port/win32env.c | 89 ++++++++++++++++++++++++++---------------------------
1 file changed, 44 insertions(+), 45 deletions(-)
diff --git a/src/port/win32env.c b/src/port/win32env.c
index 7bccee6..60bf013 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -23,19 +23,6 @@ pgwin32_putenv(const char *envval)
char *envcpy;
char *cp;
- /*
- * Each version of MSVCRT has its own _putenv() call in the runtime
- * library.
- *
- * mingw always uses MSVCRT.DLL, but if we are in a Visual C++
- * environment, attempt to update the environment in all MSVCRT modules
- * that are currently loaded, to work properly with any third party
- * libraries linked against a different MSVCRT but still relying on
- * environment variables.
- *
- * Also separately update the system environment that gets inherited by
- * subprocesses.
- */
#ifdef _MSC_VER
typedef int (_cdecl * PUTENVPROC) (const char *);
static struct
@@ -66,7 +53,51 @@ pgwin32_putenv(const char *envval)
{ NULL, NULL, NULL }
};
int i;
+#endif /* _MSC_VER */
+
+ /*
+ * Update the process environment - to make modifications visible to child
+ * processes.
+ *
+ * Need a copy of the string so we can modify it.
+ */
+ envcpy = strdup(envval);
+ if (!envcpy)
+ return -1;
+ cp = strchr(envcpy, '=');
+ if (cp == NULL)
+ {
+ free(envcpy);
+ return -1;
+ }
+ *cp = '\0';
+ cp++;
+ if (strlen(cp))
+ {
+ /*
+ * Only call SetEnvironmentVariable() when we are adding a variable,
+ * not when removing it. Calling it on both crashes on at least
+ * certain versions of MingW.
+ */
+ if (!SetEnvironmentVariable(envcpy, cp))
+ {
+ free(envcpy);
+ return -1;
+ }
+ }
+ free(envcpy);
+ /*
+ * Each version of MSVCRT has its own _putenv() call in the runtime
+ * library.
+ *
+ * mingw always uses MSVCRT.DLL, but if we are in a Visual C++
+ * environment, attempt to update the environment in all MSVCRT modules
+ * that are currently loaded, to work properly with any third party
+ * libraries linked against a different MSVCRT but still relying on
+ * environment variables.
+ */
+#ifdef _MSC_VER
for (i = 0; rtmodules[i].modulename; i++)
{
if (rtmodules[i].putenvFunc == NULL)
@@ -102,38 +133,6 @@ pgwin32_putenv(const char *envval)
}
#endif /* _MSC_VER */
- /*
- * Update the process environment - to make modifications visible to child
- * processes.
- *
- * Need a copy of the string so we can modify it.
- */
- envcpy = strdup(envval);
- if (!envcpy)
- return -1;
- cp = strchr(envcpy, '=');
- if (cp == NULL)
- {
- free(envcpy);
- return -1;
- }
- *cp = '\0';
- cp++;
- if (strlen(cp))
- {
- /*
- * Only call SetEnvironmentVariable() when we are adding a variable,
- * not when removing it. Calling it on both crashes on at least
- * certain versions of MingW.
- */
- if (!SetEnvironmentVariable(envcpy, cp))
- {
- free(envcpy);
- return -1;
- }
- }
- free(envcpy);
-
/* Finally, update our "own" cache */
return _putenv(envval);
}
--
2.10.2.windows.1
0006-Fix-the-unload-race-as-best-we-can.patchapplication/octet-stream; name=0006-Fix-the-unload-race-as-best-we-can.patchDownload
From a331eff9c278de51d543a2fcbb83357c838fecb5 Mon Sep 17 00:00:00 2001
From: Christian Ullrich <christian.ullrich@traditionsa.lu>
Date: Wed, 30 Nov 2016 16:35:26 +0100
Subject: [PATCH 6/7] Fix the unload race as best we can.
The fix is, unfortunately, to get rid of all caching.
After fixing the load race, 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 60bf013..65523f4 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -25,32 +25,27 @@ pgwin32_putenv(const char *envval)
#ifdef _MSC_VER
typedef int (_cdecl * PUTENVPROC) (const char *);
- static struct
+ static const 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 and later */
- { NULL, NULL, 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 and later */
+ NULL
};
int i;
#endif /* _MSC_VER */
@@ -98,38 +93,23 @@ pgwin32_putenv(const char *envval)
* environment variables.
*/
#ifdef _MSC_VER
- 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 = (PUTENVPROC) 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.10.2.windows.1
On Thu, Dec 1, 2016 at 1:24 AM, Christian Ullrich <chris@chrullrich.net> wrote:
* From: Michael Paquier
With 0005 I am seeing a compilation failure: you need to have the
declarations in the _MSC_VER block at the beginning of the routine. ItSorry, too used to C++.
would be nice to mention in a code comment that this what Noah has
mentioned upthread: if a CRT loads while pgwin32_putenv() is
executing, the newly-loaded CRT will always have the latest value.I fixed the existing comment by removing the last sentence that is in the wrong place now, but I don't see the point in suddenly starting to explain why it is done this way and not the other.
Based on that 0006 will need a rebase, nothing huge though.
Done, new revisions attached.
Okay, switched as ready for committer.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Nov 30, 2016 at 2:56 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Tue, Nov 29, 2016 at 08:45:13PM +0100, Christian Ullrich wrote:
* Michael Paquier wrote:
On Wed, Nov 16, 2016 at 12:45 PM, Christian Ullrich
<chris@chrullrich.net> wrote:I also did a debug build with 1+2+4 that came in at 84 μs/iteration.
Moved to next CF. Christian, perhaps this patch should have an extra
round of reviews?It is significantly different from the last version, so yes, of course.
Patches 0001 (Cosmetic fixes), 0002 (Remove unnecessary CloseHandle)
and 0003 (support for debug CRTs) look in good shape to me. 0004 (Fix
load race) is 0002 from the previous set, and this change makes sense
in itself.
0001 looks fine insofar as it makes things consistent regarding 0 vs.
NULL, but the whitespace changes will be reverted by pgindent. (I
just tested.)
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Nov 16, 2016 at 08:45:20PM +0000, Christian Ullrich wrote:
* Noah Misch wrote:
I prefer the simplicity of abandoning the cache (patch 4), if it
performs decently. Would you compare the performance of patch 1,
patches 1+2+3, and patches 1+2+4? This should measure the right
thing (after substituting @libdir@ for your environment):Tested with release builds; Core i7-6700K (quad/HT; 4000 MHz).
I did three runs each, and they were always within 0.5 % of each
other's run time.- patch 1 (now 1-3): 24 μs/iteration (24 s for 1e6)
- patch 1+2+3 (now 1-5 + 8): 29 μs/iteration
- patch 1+2+4 (now 1-7): 30 μs/iteration
Thanks for measuring; 6μs*9=54μs is a negligible addition to Windows backend
startup time.
On Wed, Nov 30, 2016 at 04:24:34PM +0000, Christian Ullrich wrote:
* From: Michael Paquier
would be nice to mention in a code comment that this what Noah has
mentioned upthread: if a CRT loads while pgwin32_putenv() is
executing, the newly-loaded CRT will always have the latest value.I fixed the existing comment by removing the last sentence that is in the wrong place now, but I don't see the point in suddenly starting to explain why it is done this way and not the other.
Based on that 0006 will need a rebase, nothing huge though.
Done, new revisions attached.
I committed patches 1-7 with some comment changes, a pgindent, and other
cosmetic trivia. (The file was pgindent-clean before this work. Patch 6
still achieved the more-compact formatting you sought.)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Dec 4, 2016 at 5:58 AM, Noah Misch <noah@leadboat.com> wrote:
On Wed, Nov 30, 2016 at 04:24:34PM +0000, Christian Ullrich wrote:
* From: Michael Paquier
would be nice to mention in a code comment that this what Noah has
mentioned upthread: if a CRT loads while pgwin32_putenv() is
executing, the newly-loaded CRT will always have the latest value.I fixed the existing comment by removing the last sentence that is in the wrong place now, but I don't see the point in suddenly starting to explain why it is done this way and not the other.
Based on that 0006 will need a rebase, nothing huge though.
Done, new revisions attached.
I committed patches 1-7 with some comment changes, a pgindent, and other
cosmetic trivia. (The file was pgindent-clean before this work. Patch 6
still achieved the more-compact formatting you sought.)
Thanks all for helping in closing this item. We have a fine result here.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers