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

* 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