Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Michael Paquier wrote:


On Tue, Sep 6, 2016 at 5:36 PM, Christian Ullrich  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

Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Michael Paquier]

On Tue, Sep 6, 2016 at 5:36 PM, Christian Ullrich  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

Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Craig Ringer]

On 6 Sep. 2016 15:12, "Michael Paquier"  wrote:
>
> On Thu, Sep 1, 2016 at 4:03 PM, Christian Ullrich 
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.

Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Michael Paquier wrote:


On Thu, Sep 1, 2016 at 4:03 PM, Christian Ullrich  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

>From dfbe7384b309c20d7733b0d18e6819302a483d95 Mon Sep 17 00:00:00 2001
From: Christian Ullrich 
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;
+   BOOLpinned;
}   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,   

Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Michael Paquier]

On Thu, Sep 1, 2016 at 4:03 PM, Christian Ullrich  wrote:
> * Michael Paquier wrote:
>
>> On Wed, Apr 27, 2016 at 2:39 AM, Christian Ullrich 
>> 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,
+   );
+   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 
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 

Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Michael Paquier wrote:

> On Wed, Apr 27, 2016 at 2:39 AM, Christian Ullrich 
 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.
> [...]
> #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?

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

Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Michael Paquier]

On Wed, Apr 27, 2016 at 2:39 AM, 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
>  \
>   `- 0004
>
> 0001 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

Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* 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 
() that contains two DLLs:


- The first one is built with VS 2013 (debug), as is the server. It
  does not matter what it is built with, except it must not be the same
  as the second DLL. It exports a single function callable from SQL.

- The second one is built with VS 2015 (debug), and again, the exact
  CRT is not important as long as it is not the same as the server
  or the first DLL.

The function does this:

1. It loads the second DLL. This brings in ucrtbased.dll as well.
2. It calls putenv().
3. It unloads the second DLL. This also causes ucrtbased.dll to be
   unloaded because it is not needed anymore.
4. It calls putenv() again.

   - With current master, this works, because pgwin32_putenv(),
 after the first call somewhere early during backend startup,
 never looks for ucrtbased again (including in step 2).

   - With patch 0002 applied, it crashes because pgwin32_putenv(),
 having detected ucrtbased.dll and cached its HMODULE during
 the call in step 2 above, reuses these values after the DLL
 is long gone.

   - With patch 0004 applied as well, it works again because no
 caching is done anymore.

Even with patch 0004, there is still a race condition between the main 
thread and a theoretical additional thread created by some other 
component that unloads some CRT between GetProcAddress() and the 
_putenv() call, but that is hardly fixable.


The fact that master looks like it does means that there have not been 
many (or any) complaints about missing cross-module environment 
variables. If nobody ever needs to see a variable set elsewhere, we have 
a very simple solution: Why don't we simply throw out the whole #ifdef 
_MSC_VER block?


There is another possible fix, ugly as sin, if we really need to keep 
the whole environment update machinery *and* cannot do the full loop 
each time. Patch 0003 pins each CRT when we see it for the first time. 
GET_MODULE_HANDLE_EX_FLAG_PIN is documented as "The module stays loaded 
until the process is terminated, no matter how many times FreeLibrary is 
called", so the unload race cannot occur anymore.


--
Christian

From d74e778d8abbfea244bacbeb352cadc737032e85 Mon Sep 17 00:00:00 2001
From: Christian Ullrich 
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
-   },  /* 

Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* 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

Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* 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
 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

Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Andrew Dunstan]

On 04/25/2016 09:27 AM, Christian Ullrich wrote:

* Magnus Hagander wrote:

On Sun, Apr 24, 2016 at 9:56 PM, Christian Ullrich 


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.




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

Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Magnus Hagander wrote:


On Sun, Apr 24, 2016 at 9:56 PM, Christian Ullrich 
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

Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Magnus Hagander]

On Sun, Apr 24, 2016 at 9:56 PM, Christian Ullrich 
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
>> --
>> master
>>
>> Details
>> ---
>>
>> 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/

Re: [COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Magnus Hagander wrote:


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


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

[COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Magnus Hagander]

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

[COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Magnus Hagander]

Add putenv support for msvcrt from Visual Studio 2013

This was missed when VS 2013 support was added.

Michael Paquier

Branch
--
REL9_4_STABLE

Details
---
http://git.postgresql.org/pg/commitdiff/c238a41014db7f818982641a8b95fae15515c88e

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

[COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Magnus Hagander]

Add putenv support for msvcrt from Visual Studio 2013

This was missed when VS 2013 support was added.

Michael Paquier

Branch
--
REL9_5_STABLE

Details
---
http://git.postgresql.org/pg/commitdiff/409c49c64a0549c39044ed6ea845ba0fb6c6190d

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

[COMMITTERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Magnus Hagander]

Add putenv support for msvcrt from Visual Studio 2013

This was missed when VS 2013 support was added.

Michael Paquier

Branch
--
REL9_3_STABLE

Details
---
http://git.postgresql.org/pg/commitdiff/ab5c6d01f6dbe036469fd77b488a21ca8d7d26f1

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