Обсуждение: Can a windows DLL have more than one process attached?

In reviewing some recent patches I notice the following code added to
src/interfaces/libpq/libpqdll.c:

BOOL WINAPI
DllMain(HINSTANCE hinstDLL, DWORD fdwReason,       LPVOID lpReserved)
{   switch (fdwReason)   {       case DLL_PROCESS_ATTACH:           ...           if (netmsgModule == NULL)
 netmsgModule = LoadLibraryEx("netmsg.dll", NULL, LOAD_LIBRARY_AS_DATAFILE);           break;       case
DLL_PROCESS_DETACH:          if (netmsgModule != NULL)               FreeLibrary(netmsgModule);           ...
break;  }
 
}

where netmsgModule is

static HINSTANCE netmsgModule = NULL;

This sure looks to me like it will fail miserably if more than one
process can attach to the DLL concurrently: won't the first one to
detach release the netmsg library, breaking access to it for all the
remaining processes?

I don't know enough about Windows to know if there's really a problem
here, but the code looks fishy to me.  I'd expect to need a reference
count.  Comments anyone?
        regards, tom lane

On Tue, Nov 27, 2001 at 01:31:40PM -0500, Tom Lane wrote:
> In reviewing some recent patches I notice the following code added to
> src/interfaces/libpq/libpqdll.c:
> 
> BOOL WINAPI
> DllMain(HINSTANCE hinstDLL, DWORD fdwReason,
>         LPVOID lpReserved)
> {
>     switch (fdwReason)
>     {
>         case DLL_PROCESS_ATTACH:
>             ...
>             if (netmsgModule == NULL)
>                 netmsgModule = LoadLibraryEx("netmsg.dll", NULL, LOAD_LIBRARY_AS_DATAFILE);
>             break;
>         case DLL_PROCESS_DETACH:
>             if (netmsgModule != NULL)
>                 FreeLibrary(netmsgModule);
>             ...
>             break;
>     }
> }
> 
> where netmsgModule is
> 
> static HINSTANCE netmsgModule = NULL;
> 
> This sure looks to me like it will fail miserably if more than one
> process can attach to the DLL concurrently: won't the first one to
> detach release the netmsg library, breaking access to it for all the
> remaining processes?
> 
> I don't know enough about Windows to know if there's really a problem
> here, but the code looks fishy to me.  I'd expect to need a reference
> count.  Comments anyone?

I'm not much of a WinAPI coder, either, but ISTR something from the Wine
project, and found this:

http://www.allapi.net/apilist/apifunction.php?apifunction=FreeLibrary
FreeLibrary
The FreeLibrary function decrements the reference count of theloaded dynamic-link library (DLL) module.

So it's doing the reference counting.

'Offical' reference at:

http://msdn.microsoft.com/library/en-us/dllproc/dll_3cs9.asp?frame=false

However, there's another problem. According to the above, and:

http://msdn.microsoft.com/library/en-us/dllproc/dll_4abc.asp?frame=true

It is not safe to call FreeLibrary or LoadLibraryEx from DllMain.

Which points to:

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/dll_8asu.asp
Warning  On attach, the body of your DLL entry-point functionshould perform only simple initialization tasks, such
assettingup thread local storage (TLS), creating objects, andopening files. You must not call LoadLibrary  in the
entry-pointfunction,because you may create dependency loops in the DLL loadorder. This can result in a DLL being used
beforethe systemhas executed its initialization code. Similarly, you must notcall the FreeLibrary  function in the
entry-pointfunction ondetach, because this can result in a DLL being used after thesystem has executed its termination
code.


Any real windows coders know if this is a problem?

Ross

"Ross J. Reedstrom" <reedstrm@rice.edu> writes:
>     The FreeLibrary function decrements the reference count of the
>     loaded dynamic-link library (DLL) module.
> So it's doing the reference counting.

Yeah, but what increments the reference count?  I could believe the code
was okay if it did LoadLibraryEx (which presumably increments the count)
on *every* attach call, but it does that only once, whereas it'll do
FreeLibrary on each detach.

However, we may have worse problems.

> It is not safe to call FreeLibrary or LoadLibraryEx from DllMain.

This looks ugly, although I'm not sure if it's really a problem for us.
I don't see how our pointing to netmsg could create a dependency loop.

I'm also wondering just exactly how Microsoft expects one DLL to be able
to safely reference another, if it's unsafe to call FreeLibrary during
detach --- what else are you supposed to do, leak the reference count?

If this is a problem, a possible answer is not to try to cache the
netmsg reference at all, but just to load and unload that DLL at the
single point where it's used.  Since we only use it to translate
socket error reports, there's probably no big performance penalty
involved to do it that way.
        regards, tom lane

On Tue, Nov 27, 2001 at 01:01:47PM -0600, Ross J. Reedstrom wrote:
> 
> However, there's another problem. According to the above, and:
> 
> http://msdn.microsoft.com/library/en-us/dllproc/dll_4abc.asp?frame=true
> 
> It is not safe to call FreeLibrary or LoadLibraryEx from DllMain.
> 
> Which points to:
> 
> http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/dll_8asu.asp
> 
>     Warning  On attach, the body of your DLL entry-point function
>     should perform only simple initialization tasks, such as
>     setting up thread local storage (TLS), creating objects, and
>     opening files. You must not call LoadLibrary  in the entry-point
>     function, because you may create dependency loops in the DLL load
>     order. This can result in a DLL being used before the system
>     has executed its initialization code. Similarly, you must not
>     call the FreeLibrary  function in the entry-point function on
>     detach, because this can result in a DLL being used after the
>     system has executed its termination code.
> 
> 
> Any real windows coders know if this is a problem?
> 

Reading into this further, it seems to me that this code is attempting to
implement DLL dependencies. I'd think that the DLL loader is _supposed_
to handle that automatically (based on te docmuentation for LoadLibrary
mentioning that it can cause dependent libraries to also be loaded), but
I know even less about building win32 dlls than I do about coding them.
Are we mis-building somehow?

The warning above also sounds like the only problem is reference loops.

Ross

"Ross J. Reedstrom" <reedstrm@rice.edu> writes:
> Reading into this further, it seems to me that this code is attempting to
> implement DLL dependencies. I'd think that the DLL loader is _supposed_
> to handle that automatically (based on te docmuentation for LoadLibrary
> mentioning that it can cause dependent libraries to also be loaded), but
> I know even less about building win32 dlls than I do about coding them.

Ditto.  But, looking at the cvs logs, it seems that the use of netmsg
was inserted rather hastily, and it may well be done completely the
wrong way.

I'm not sure that declaring a dependency to the DLL loader is the way we
want to go though; wouldn't that cause the system to refuse to run libpq
at all unless netmsg was available?  That's certainly not the behavior
we want, considering that netmsg is only a fallback source of error
strings in the first place, and that we can fall back to "no error text
available" if we can't load it.

Loading and unloading on-the-fly in winsock_strerror is looking better
and better to me.
        regards, tom lane

Actually, the more I look at this, the more broken it looks.

(1) netmsgModule and winsock_strerror_buf are declared as static
variables in libpq/win32.h.  Unless Windows has some truly creative new
interpretation of static variables, this means that there'll be a
separate instance of these vars in each .c file that #includes win32.h.
Wasting half a K of space per .c file is the least of the consequences.

(2) Since libpqdll.c and fe-misc.c don't share the same instance of
netmsgModule, winsock_strerror will always see it as NULL regardless
of what DllMain has done or not done.

(3) Even if winsock_strerror saw netmsgModule, it doesn't pass it to
FormatMessage, which means (if I'm reading the documentation I found on
the net correctly) that the wrong message table would be searched.
I think that the "0" parameter should be netmsgModule instead.

Have we got any people who actually know the Windows platform and
can confirm or deny these points?

I'd be happy to change the code to something that looks reasonable to
me, but I'm in no position to test it.
        regards, tom lane

> > It is not safe to call FreeLibrary or LoadLibraryEx from DllMain.
> 
> This looks ugly, although I'm not sure if it's really a problem for
us.
> I don't see how our pointing to netmsg could create a dependency loop.
Shouldn't happen in this case, no - it's probably a generic warning
message... Still, it's not recommended behaviour...


> I'm also wondering just exactly how Microsoft expects one DLL 
> to be able
> to safely reference another, if it's unsafe to call FreeLibrary during
> detach --- what else are you supposed to do, leak the reference count?
If you reference the library during compilation (e.g. you link with the
.LIB file that corresponds to the .DLL), the OS will handle that as part
of the CreateProcess() call.


As for the FreeLibrary problem, you also have to deal with:
There are cases in which the entry-point function is called for a
terminating thread even if the DLL never attached to the thread - for
example, the entry-point function was never called with the
DLL_THREAD_ATTACH value in the context of the thread in either of these
two situations: 
* The thread was the initial thread in the process, so the system called
the entry-point function with the DLL_PROCESS_ATTACH value. 
* The thread was already running when a call to the LoadLibrary function
was made, so the system never called the entry-point function for it. 


In general, depending on doing things in DllMain is not a good idea.
The fact that we do WSAStartup() in it is even deprecated in the docs
now, for the same reason: "Calling Win32 functions other than TLS,
object-creation, and file functions may result in problems that are
difficult to diagnose. For example, calling User, Shell, COM, RPC, and
Windows Sockets functions (or any functions that call these functions)
can cause access violation errors, because their DLLs call LoadLibrary
to load other system components".


The question is if it should be moved to the connection functions, or if
it shuold be left up to the application to do it. Moving it to the
connection functino would be easy, but each call to WSAStartup() should
have a corresponding WSACleanup() call...
Or if we should just leave it :-) Because it appears to work...

> If this is a problem, a possible answer is not to try to cache the
> netmsg reference at all, but just to load and unload that DLL at the
> single point where it's used.  Since we only use it to translate
> socket error reports, there's probably no big performance penalty
> involved to do it that way.
That's probably the best idea.


> (1) netmsgModule and winsock_strerror_buf are declared as static
> variables in libpq/win32.h.  Unless Windows has some truly creative
new
> interpretation of static variables, this means that there'll be a
> separate instance of these vars in each .c file that #includes
win32.h.
> Wasting half a K of space per .c file is the least of the
consequences.
static is static in Windows as well (it is C, after all...) If it's
declared static in the header file, there will be a separate instance in
each C file.

> I'd be happy to change the code to something that looks reasonable to
> me, but I'm in no position to test it.
I can probably do some testing - assuming the 7.2 client libs can still
talk to 7.1 servers, since I haven't any servers up on the new version
yet...

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:
> In general, depending on doing things in DllMain is not a good idea.
> The fact that we do WSAStartup() in it is even deprecated in the docs
> now, ...
> The question is if it should be moved to the connection functions, or if
> it shuold be left up to the application to do it. Moving it to the
> connection functino would be easy, but each call to WSAStartup() should
> have a corresponding WSACleanup() call...
> Or if we should just leave it :-) Because it appears to work...

Well, it's been that way since day one and we've not had any reports of
problems, so I'm inclined to leave it alone until we see a problem.

However, this netmsg.dll reference is new code in 7.2 and is evidently
not well tested :-(.

>> If this is a problem, a possible answer is not to try to cache the
>> netmsg reference at all, but just to load and unload that DLL at the
>> single point where it's used.  Since we only use it to translate
>> socket error reports, there's probably no big performance penalty
>> involved to do it that way.

> That's probably the best idea.

Okay, I'll make the changes and then ask you to test it.  Do you have a
copy of netmsg.dll?

> I can probably do some testing - assuming the 7.2 client libs can still
> talk to 7.1 servers, since I haven't any servers up on the new version
> yet...

Should work fine.  But the code we're interested in testing here is a
fallback code path --- it only gets exercised if the main system doesn't
have a string for the error message.  You might need to dike out the
first FormatMessage call in winsock_strerror in order to test.
        regards, tom lane

> where netmsgModule is
> 
> static HINSTANCE netmsgModule = NULL;
> 

That is definitely my bug (cut&paste problem:(). There shouldn't be a
static variable in *.h files. Sorry :(.


> This sure looks to me like it will fail miserably if more than one
> process can attach to the DLL concurrently: won't the first one to
> detach release the netmsg library, breaking access to it for all the
> remaining processes?
> 

There is no problem here - LoadLibrary and FreeLibrary use reference
count (see references in the Ross's message).


> I don't know enough about Windows to know if there's really a problem
> here, but the code looks fishy to me.  I'd expect to need a reference
> count.  Comments anyone?
> 
Windows really does reference count here.


Regards,
Mikhail

 
> http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/dll_8asu.asp
> 
>     Warning  On attach, the body of your DLL entry-point function
>     should perform only simple initialization tasks, such as
>     setting up thread local storage (TLS), creating objects, and
>     opening files. You must not call LoadLibrary  in the entry-point
>     function, because you may create dependency loops in the DLL load
>     order. This can result in a DLL being used before the system
>     has executed its initialization code. Similarly, you must not
>     call the FreeLibrary  function in the entry-point function on
>     detach, because this can result in a DLL being used after the
>     system has executed its termination code.
> 

I believe that in this particular case we are safe because we load netmsg.dll
as data (LOAD_LIBRARY_AS_DATAFILE).

Regards,
Mikhail

Tom Lane wrote:

> "Ross J. Reedstrom" <reedstrm@rice.edu> writes:
>>    The FreeLibrary function decrements the reference count of the
>>    loaded dynamic-link library (DLL) module.
>>So it's doing the reference counting.
>>
> 
> Yeah, but what increments the reference count?  I could believe the code
> was okay if it did LoadLibraryEx (which presumably increments the count)
> on *every* attach call, but it does that only once, whereas it'll do
> FreeLibrary on each detach.


LoadLibrary increments reference count and FreeLibrary decrements it on every
call not just once.

> 
> However, we may have worse problems.
> 
> 
>>It is not safe to call FreeLibrary or LoadLibraryEx from DllMain.
>>
> 
> This looks ugly, although I'm not sure if it's really a problem for us.

What is the ugliness? We load this data only once at startup and release

on exit.


> I don't see how our pointing to netmsg could create a dependency loop.


There shouldn't be a dependency loop - we load netmsg as data not a code library.

> 
> If this is a problem, a possible answer is not to try to cache the
> netmsg reference at all, but just to load and unload that DLL at the
> single point where it's used.  Since we only use it to translate


That is really ugly in my opinion.

> socket error reports, there's probably no big performance penalty
> involved to do it that way.
> 

Regards,
Mikhail

Magnus Hagander wrote:

>
> If you reference the library during compilation (e.g. you link with the
> .LIB file that corresponds to the .DLL), the OS will handle that as part
> of the CreateProcess() call.
> 


What if the required DLL is missing (as is the case with netmsg.dll) on some
system?

> 
> As for the FreeLibrary problem, you also have to deal with:
> There are cases in which the entry-point function is called for a
> terminating thread even if the DLL never attached to the thread - for
> example, the entry-point function was never called with the
> DLL_THREAD_ATTACH value in the context of the thread in either of these
> two situations: 
> * The thread was the initial thread in the process, so the system called
> the entry-point function with the DLL_PROCESS_ATTACH value. 
> * The thread was already running when a call to the LoadLibrary function
> was made, so the system never called the entry-point function for it. 
> 


That is why it is better to do such a global initialization/finalization on
DLL_PROCESS_ATTACH/DETACH.


> 
> In general, depending on doing things in DllMain is not a good idea.
> The fact that we do WSAStartup() in it is even deprecated in the docs
> now, for the same reason: "Calling Win32 functions other than TLS,
> object-creation, and file functions may result in problems that are
> difficult to diagnose. For example, calling User, Shell, COM, RPC, and
> Windows Sockets functions (or any functions that call these functions)
> can cause access violation errors, because their DLLs call LoadLibrary
> to load other system components".
> 


In netmsg.dll case it looks very like "file function" because we load it as data.
In case of WSAStartup() consider the following scenario:
Application doesn't load libpq.dll explicitly, OS loads it during CreateProcess()
call. Suppose there is some sockets problem exists. What happens with
the application? Will it start up at all? What if this application is
still useful even without connection to DB?

> 
> The question is if it should be moved to the connection functions, or if
> it shuold be left up to the application to do it. Moving it to the
> connection functino would be easy, but each call to WSAStartup() should
> have a corresponding WSACleanup() call...
> Or if we should just leave it :-) Because it appears to work...
> 


The rcommended way is to have initialization/finalization functions in DLL.

> 
>>If this is a problem, a possible answer is not to try to cache the

>>netmsg reference at all, but just to load and unload that DLL at the
>>single point where it's used.  Since we only use it to translate
>>socket error reports, there's probably no big performance penalty
>>involved to do it that way.
>>
> That's probably the best idea.
> 
> 
> 
>>(1) netmsgModule and winsock_strerror_buf are declared as static
>>variables in libpq/win32.h.  Unless Windows has some truly creative
>>

That was my copy/paste bug, sorry again.


Regards,
Mikhail

Mikhail Terekhov wrote:
> 
> Magnus Hagander wrote:
> 
> >
> >>(1) netmsgModule and winsock_strerror_buf are declared as static
> >>variables in libpq/win32.h.  Unless Windows has some truly creative
> >>
> 
> That was my copy/paste bug, sorry again.

I've not followed this thread properly sorry.
What's the purpose of loading netmsg.dll ?
I got no error message for winsock errors under Windows-NT
using the module.

regards,
Hiroshi Inoue

Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> What's the purpose of loading netmsg.dll ?

To get error messages under some flavors of Windows; I forget which.

> I got no error message for winsock errors under Windows-NT
> using the module.

The code as it stood was broken (static variable problem: the handle
opened by libpqdll.c was not visible in fe-misc.c).  Would
you update from CVS and see if it works better?
        regards, tom lane

Tom Lane wrote:
> 
> Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> 
> > I got no error message for winsock errors under Windows-NT
> > using the module.
> 
> The code as it stood was broken (static variable problem: the handle
> opened by libpqdll.c was not visible in fe-misc.c).  Would
> you update from CVS and see if it works better?

I changed it by myself and have tested it since yesterday.
I also tested a simple sample program. However I've got no
message for winsock errors under Windows-NT 4.0. OTOH under
Windows 2000 I can get error messages for winsock errors
without loading netmsg.dll.

regards,
Hiroshi Inoue

Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> I changed it by myself and have tested it since yesterday.
> I also tested a simple sample program. However I've got no
> message for winsock errors under Windows-NT 4.0.

It appeared to me that the parameters passed to FormatMessage
were wrong, too (the handle for netmsg.dll needs to be passed in,
I'd think).  Perhaps that explains why it still doesn't work for
you?
        regards, tom lane

Tom Lane wrote:
> 
> Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> > I changed it by myself and have tested it since yesterday.
> > I also tested a simple sample program. However I've got no
> > message for winsock errors under Windows-NT 4.0.
> 
> It appeared to me that the parameters passed to FormatMessage
> were wrong, too (the handle for netmsg.dll needs to be passed in,
> I'd think).  

I did so from the first. I also tried the latest cvs version
but the result is the same. ISTM Mikhail Terekhov referred to
the fact 3 months ago but I couldn't find the conclusion.

regards,
Hiroshi Inoue

Hiroshi Inoue wrote:

> I did so from the first. I also tried the latest cvs version
> but the result is the same. ISTM Mikhail Terekhov referred to
> the fact 3 months ago but I couldn't find the conclusion.
> 

Do you really have this DLL on your system?

Regards,
Mikhail

After refreshing my memory I can say for sure that it never
worked on nt4 and w98, see
http://fts.postgresql.org/db/mw/msg.html?mid=1030825

The reason is very simple: on nt4 & w98 (don't know about 95)
netmsg.dll doesn't contain socket error messages! I just checked
this on my work computer nt4(sp6).

Regards,
Mikhail

Hiroshi Inoue wrote:

> Tom Lane wrote:
> 
>>Hiroshi Inoue <Inoue@tpf.co.jp> writes:
>>
>>>I changed it by myself and have tested it since yesterday.
>>>I also tested a simple sample program. However I've got no
>>>message for winsock errors under Windows-NT 4.0.
>>>
>>It appeared to me that the parameters passed to FormatMessage
>>were wrong, too (the handle for netmsg.dll needs to be passed in,
>>I'd think).  
>>
> 
> I did so from the first. I also tried the latest cvs version
> but the result is the same. ISTM Mikhail Terekhov referred to
> the fact 3 months ago but I couldn't find the conclusion.
> 
> regards,
> Hiroshi Inoue
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
> 

Mikhail Terekhov <terekhov@emc.com> writes:
> The reason is very simple: on nt4 & w98 (don't know about 95)
> netmsg.dll doesn't contain socket error messages! I just checked
> this on my work computer nt4(sp6).

Ooops.  Well, are there any WINxx platforms where netmsg *does* have
something useful?  Or is the entire code path a waste of time?

Is there some other DLL we could look in instead?
        regards, tom lane

> Mikhail Terekhov <terekhov@emc.com> writes:
> > The reason is very simple: on nt4 & w98 (don't know about 95)
> > netmsg.dll doesn't contain socket error messages! I just checked
> > this on my work computer nt4(sp6).
> 
> Ooops.  Well, are there any WINxx platforms where netmsg *does* have
> something useful?  Or is the entire code path a waste of time?

Never checked this before, but according to:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/netmgmt
/ntlmapi_5pwz.asp
(from the latest platform SDK - I think)
and
http://support.microsoft.com/directory/article.asp?ID=KB;EN-US;Q149409
(which references NT 3.51, so it's pretty old, but *probably* still
valid..)

It clearly states:
"
The error text for these messages is found in the message table file
named Netmsg.dll, which is found in %systemroot%\system32. This file
contains error messages in the range NERR_BASE (2100) through
MAX_NERR(NERR_BASE+899). These error codes are defined in the SDK header
file lmerr.h.
"

This means errors for the Net*** functions, which is the LAN Manager
client and server. This is *NOT* Windows Sockets. (And Winsock errors
are at >10000, so clearly does not fall withing
NERR_BASE<=n<=NERR_BASE+899).


So if that documentation is correct, then the entire path should be a
waste of time. But perhaps the original author has some other reference
that says it actually works with Winsock?


> Is there some other DLL we could look in instead?
I can't find a reference to one anywhere - except it's included by
default in at least Win2k and later (with no extra loading of DLLs)


//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:
>> Is there some other DLL we could look in instead?

> I can't find a reference to one anywhere - except it's included by
> default in at least Win2k and later (with no extra loading of DLLs)

Well, we're down to the wire on 7.2, and I don't think there will be
any votes to postpone 7.2 release while we resolve this problem ;-).
So what I propose to do is remove the netmsg.dll code for now,
leaving only the FormatMessage(...FORMAT_MESSAGE_FROM_SYSTEM...) call
in winsock_strerror.  This is still better than what we had in 7.1.
We can look for a better answer for older Windows flavors to add in
7.3 or later.

Any objections?
        regards, tom lane