Thread: WSAStartup() in libpq
It has been brought to my attention by Tokuharu Yuzawa that our calling of WSAStartup() in DllMain() (libpqdll.c) is incorrect. Basically we're calling WSAStartup() so that the client application does not have to. However, due to the fact that WSAStartup() can itself load libraries, there is a risk of deadlocking here. See for example: MSDN docs http://msdn2.microsoft.com/en-us/library/aa910684.aspx MIT Kerberos have had the same problem: http://mailman.mit.edu/pipermail/krbdev/2005-March/003244.html And even MS had the same bug in their own native database library: http://support.microsoft.com/kb/818539 There's also a note about socket issues in DLLs on: http://support.microsoft.com/kb/q237572/ The easy fix for this is to remove the calls. Which obviously will break some client apps. A fairly easy fix for the WSAStartup() call is to have a check in the connection functions against a global variable that will then make sure to call WSAStartup() the first time it's called. That would leave us "leaking" the WSAStartup() call, but only one per application. This is not perfect, but I'm thinking we can maybe live with that. If not, perhaps we can have it call WSAStartup() everytime we connect to a server, and then WSACleanup() when we shut down that connection with PQfinish(). We're still going to leak if the user forgets to run PQfinish(), but we're leaking other resources as well in that case. That will break if any network related functions are called when there is no connection open, but I doubt that's possible? Of course, if we had a libpq_init() and a libpq_shutdown() function things would be very easy, but we don't. And adding it just for this purpose seems like trying too hard. Yet another option would be to require that the client app deal with the startup/shutdown code itself, but that will seriously break backwards compatibility, so I don't think that's a good idea at all. Other ideas? //Magnus
Magnus Hagander wrote: > > The easy fix for this is to remove the calls. Which obviously will break > some client apps. A fairly easy fix for the WSAStartup() call is to have > a check in the connection functions against a global variable that will > then make sure to call WSAStartup() the first time it's called. > > That would leave us "leaking" the WSAStartup() call, but only one per > application. This is not perfect, but I'm thinking we can maybe live > with that. > > If not, perhaps we can have it call WSAStartup() everytime we connect to > a server, and then WSACleanup() when we shut down that connection with > PQfinish(). Taken from MSDN docs, this seems the recommended solution. After the first WSAStartup call subsequent calls are cheap because they only increment a counter. Regards, Andreas
On Thu, Mar 08, 2007 at 12:47:42PM +0100, Andreas Pflug wrote: > Magnus Hagander wrote: > > > > The easy fix for this is to remove the calls. Which obviously will break > > some client apps. A fairly easy fix for the WSAStartup() call is to have > > a check in the connection functions against a global variable that will > > then make sure to call WSAStartup() the first time it's called. > > > > That would leave us "leaking" the WSAStartup() call, but only one per > > application. This is not perfect, but I'm thinking we can maybe live > > with that. > > > > If not, perhaps we can have it call WSAStartup() everytime we connect to > > a server, and then WSACleanup() when we shut down that connection with > > PQfinish(). > > Taken from MSDN docs, this seems the recommended solution. After the > first WSAStartup call subsequent calls are cheap because they only > increment a counter. Now that I look closer at it, we *already* do WSAStartup() in makeEmptyPGconn... And free it in freePGconn(). So I suggest the following simple patch.. Any objections? //Magnus
Attachment
Magnus Hagander <magnus@hagander.net> writes:
> So I suggest the following simple patch.. Any objections?
One wonders if we need DllMain() at all any more. We certainly don't
need that switch statement ...
Also, isn't the WSACleanup() in freePGconn in the wrong place? Seems
like it shouldn't be done until after we've closed the socket. I'd
be inclined to put it at the bottom of the routine.
regards, tom lane
On Thu, Mar 08, 2007 at 10:10:28AM -0500, Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: > > So I suggest the following simple patch.. Any objections? > > One wonders if we need DllMain() at all any more. We certainly don't > need that switch statement ... Indeed. Looking even more into it (sheesh, I really didn't do my homework here), libpqdll.c isn't even *compiled* on mingw. Or on the new MSVC build. It's only compiled on the old msvc build. Given that, we can probably just delete the file. > Also, isn't the WSACleanup() in freePGconn in the wrong place? Seems > like it shouldn't be done until after we've closed the socket. I'd > be inclined to put it at the bottom of the routine. Certainly looks wrong. It's interesting how this could have worked *before*. That's a clear indication that it really doesn't appear to matter much what we do here :S The patch would then look something like this, and a remove of libpqdll.c. //Magnus
Attachment
Magnus Hagander <magnus@hagander.net> writes:
> On Thu, Mar 08, 2007 at 10:10:28AM -0500, Tom Lane wrote:
>> Also, isn't the WSACleanup() in freePGconn in the wrong place? Seems
>> like it shouldn't be done until after we've closed the socket. I'd
>> be inclined to put it at the bottom of the routine.
> Certainly looks wrong. It's interesting how this could have worked
> *before*.
Because the calls in DllMain covered us (ie, the WSA usage count never
got to be less than one). If we remove them, we'd better get this pair
right.
One thing that bothers me a bit is that if we just move the call to the
bottom, then freePGconn won't do it at all if passed a NULL pointer.
Now (assuming a non-broken app) the only way that can happen is if
makeEmptyPGconn runs out of memory. If the client gets back a null
pointer from a connection attempt, it's probably a 50-50 proposition
whether it will think it ought to do PQfinish with it. So it'd be good
if we could keep the usage count straight either way. I propose the
invariant "a WSA usage count is associated with a non-null PGconn
structure". That would mean that doing WSACleanup only at the bottom
of freePGconn is correct, but also that makeEmptyPGconn needs to do
WSACleanup in its (two) failure paths.
regards, tom lane
On Thu, Mar 08, 2007 at 10:37:11AM -0500, Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: > > On Thu, Mar 08, 2007 at 10:10:28AM -0500, Tom Lane wrote: > >> Also, isn't the WSACleanup() in freePGconn in the wrong place? Seems > >> like it shouldn't be done until after we've closed the socket. I'd > >> be inclined to put it at the bottom of the routine. > > > Certainly looks wrong. It's interesting how this could have worked > > *before*. > > Because the calls in DllMain covered us (ie, the WSA usage count never > got to be less than one). If we remove them, we'd better get this pair > right. But those calls weren't even compiled in when building using mingw, which is what the majority of our users have been using lately, I think. (Since that's what we ship in the binary package) > One thing that bothers me a bit is that if we just move the call to the > bottom, then freePGconn won't do it at all if passed a NULL pointer. > Now (assuming a non-broken app) the only way that can happen is if > makeEmptyPGconn runs out of memory. If the client gets back a null > pointer from a connection attempt, it's probably a 50-50 proposition > whether it will think it ought to do PQfinish with it. So it'd be good > if we could keep the usage count straight either way. I propose the > invariant "a WSA usage count is associated with a non-null PGconn > structure". That would mean that doing WSACleanup only at the bottom > of freePGconn is correct, but also that makeEmptyPGconn needs to do > WSACleanup in its (two) failure paths. I'm honestly unsure wether we need to bother with it, but yeah, that will likely be "more correct". (Except one of the error paths in makeEmptyPGconn is already covered, since it calls freePGconn, which does the WSACleanup) //Magnus
Attachment
Magnus Hagander <magnus@hagander.net> writes:
> On Thu, Mar 08, 2007 at 10:37:11AM -0500, Tom Lane wrote:
>> Because the calls in DllMain covered us (ie, the WSA usage count never
>> got to be less than one). If we remove them, we'd better get this pair
>> right.
> But those calls weren't even compiled in when building using mingw,
Hmm ... does make you wonder, doesn't it? But anyway, if we're
bothering to call the functions at all, we should try to meet the
defined protocol. So I like this latest version of the patch.
regards, tom lane