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

* 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