On Thu, Sep 1, 2016 at 4:03 PM, Christian Ullrich <ch...@chrullrich.net> wrote:
> * Michael Paquier wrote:
>
>> On Wed, Apr 27, 2016 at 2:39 AM, Christian Ullrich <ch...@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
From b809a0b1c323529c2d7460962a3c688ad8aec3f4 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@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
From 27d47434e83d5d81b912a194624910c655729431 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 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
From 49ca06e0042caf5c5264aed91f8f38f062dacb60 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
