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
Attachments:
libpq.difftext/plain; charset=us-asciiDownload
Index: fe-connect.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.344
diff -c -r1.344 fe-connect.c
*** fe-connect.c 20 Feb 2007 15:20:51 -0000 1.344
--- fe-connect.c 8 Mar 2007 12:16:42 -0000
***************
*** 1840,1848 ****
#ifdef WIN32
/*
! * Make sure socket support is up and running. Even though this is done in
! * libpqdll.c, that is only for MSVC and BCC builds and doesn't work for
! * static builds at all, so we have to do it in the main code too.
*/
WSADATA wsaData;
--- 1840,1846 ----
#ifdef WIN32
/*
! * Make sure socket support is up and running.
*/
WSADATA wsaData;
Index: libpqdll.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/libpqdll.c,v
retrieving revision 1.10
diff -c -r1.10 libpqdll.c
*** libpqdll.c 11 Mar 2006 04:38:40 -0000 1.10
--- libpqdll.c 8 Mar 2007 12:05:59 -0000
***************
*** 15,31 ****
switch (fdwReason)
{
case DLL_PROCESS_ATTACH:
! if (WSAStartup(MAKEWORD(1, 1), &wsaData))
! {
! /*
! * No really good way to do error handling here, since we
! * don't know how we were loaded
! */
! return FALSE;
! }
break;
case DLL_PROCESS_DETACH:
! WSACleanup();
break;
}
--- 15,24 ----
switch (fdwReason)
{
case DLL_PROCESS_ATTACH:
! /* We used to call WSAStartup() here, but this may cause deadlocks */
break;
case DLL_PROCESS_DETACH:
! /* We used to call WSACleanup() here, but this may cause deadlocks */
break;
}
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
Attachments:
libpq.difftext/plain; charset=us-asciiDownload
Index: bcc32.mak
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/bcc32.mak,v
retrieving revision 1.26
diff -c -r1.26 bcc32.mak
*** bcc32.mak 11 Jan 2007 02:42:31 -0000 1.26
--- bcc32.mak 8 Mar 2007 15:23:17 -0000
***************
*** 93,99 ****
-@erase "$(INTDIR)\fe-secure.obj"
-@erase "$(INTDIR)\pqexpbuffer.obj"
-@erase "$(INTDIR)\pqsignal.obj"
- -@erase "$(OUTDIR)\libpqdll.obj"
-@erase "$(OUTDIR)\win32.obj"
-@erase "$(INTDIR)\wchar.obj"
-@erase "$(INTDIR)\encnames.obj"
--- 93,98 ----
***************
*** 155,168 ****
LINK32=ilink32.exe
LINK32_FLAGS = -Gn -L$(BCB)\lib;$(INTDIR); -x -Tpd -v
- LINK32_OBJS= "$(INTDIR)\libpqdll.obj"
# @<< is a Response file, http://www.opussoftware.com/tutorial/TutMakefile.htm
! "$(OUTDIR)\blibpq.dll": "$(OUTDIR)\blibpq.lib" $(LINK32_OBJS) "$(INTDIR)\libpq.res" blibpqdll.def
$(LINK32) @<<
$(LINK32_FLAGS) +
! c0d32.obj $(LINK32_OBJS), +
$@,, +
"$(OUTDIR)\blibpq.lib" import32.lib cw32mt.lib, +
blibpqdll.def,"$(INTDIR)\libpq.res"
--- 154,166 ----
LINK32=ilink32.exe
LINK32_FLAGS = -Gn -L$(BCB)\lib;$(INTDIR); -x -Tpd -v
# @<< is a Response file, http://www.opussoftware.com/tutorial/TutMakefile.htm
! "$(OUTDIR)\blibpq.dll": "$(OUTDIR)\blibpq.lib" "$(INTDIR)\libpq.res" blibpqdll.def
$(LINK32) @<<
$(LINK32_FLAGS) +
! c0d32.obj , +
$@,, +
"$(OUTDIR)\blibpq.lib" import32.lib cw32mt.lib, +
blibpqdll.def,"$(INTDIR)\libpq.res"
Index: fe-connect.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.344
diff -c -r1.344 fe-connect.c
*** fe-connect.c 20 Feb 2007 15:20:51 -0000 1.344
--- fe-connect.c 8 Mar 2007 15:16:58 -0000
***************
*** 1840,1848 ****
#ifdef WIN32
/*
! * Make sure socket support is up and running. Even though this is done in
! * libpqdll.c, that is only for MSVC and BCC builds and doesn't work for
! * static builds at all, so we have to do it in the main code too.
*/
WSADATA wsaData;
--- 1840,1846 ----
#ifdef WIN32
/*
! * Make sure socket support is up and running.
*/
WSADATA wsaData;
***************
*** 1918,1927 ****
PGnotify *notify;
pgParameterStatus *pstatus;
- #ifdef WIN32
- WSACleanup();
- #endif
-
if (!conn)
return;
--- 1916,1921 ----
***************
*** 1986,1991 ****
--- 1980,1989 ----
termPQExpBuffer(&conn->errorMessage);
termPQExpBuffer(&conn->workBuffer);
free(conn);
+
+ #ifdef WIN32
+ WSACleanup();
+ #endif
}
/*
Index: win32.mak
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/win32.mak,v
retrieving revision 1.43
diff -c -r1.43 win32.mak
*** win32.mak 11 Jan 2007 02:42:31 -0000 1.43
--- win32.mak 8 Mar 2007 15:22:29 -0000
***************
*** 63,69 ****
-@erase "$(INTDIR)\fe-secure.obj"
-@erase "$(INTDIR)\pqexpbuffer.obj"
-@erase "$(INTDIR)\pqsignal.obj"
- -@erase "$(OUTDIR)\libpqdll.obj"
-@erase "$(OUTDIR)\win32.obj"
-@erase "$(INTDIR)\wchar.obj"
-@erase "$(INTDIR)\encnames.obj"
--- 63,68 ----
***************
*** 143,149 ****
/pdb:"$(OUTDIR)\libpqdll.pdb" /machine:I386 /out:"$(OUTDIR)\$(OUTFILENAME).dll"\
/implib:"$(OUTDIR)\$(OUTFILENAME)dll.lib" /def:$(OUTFILENAME)dll.def
LINK32_OBJS= \
- "$(INTDIR)\libpqdll.obj" \
"$(OUTDIR)\$(OUTFILENAME).lib" \
"$(OUTDIR)\libpq.res"
--- 142,147 ----
***************
*** 159,165 ****
$(RSC) $(RSC_PROJ) libpq.rc
! "$(OUTDIR)\$(OUTFILENAME).dll" : "$(OUTDIR)" "$(OUTDIR)\libpqdll.obj" "$(INTDIR)\libpqdll.obj" "$(INTDIR)\libpq.res"
$(LINK32) @<<
$(LINK32_FLAGS) $(LINK32_OBJS)
<<
--- 157,163 ----
$(RSC) $(RSC_PROJ) libpq.rc
! "$(OUTDIR)\$(OUTFILENAME).dll" : "$(OUTDIR)" "$(INTDIR)\libpq.res"
$(LINK32) @<<
$(LINK32_FLAGS) $(LINK32_OBJS)
<<
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
Attachments:
libpq.difftext/plain; charset=us-asciiDownload
Index: bcc32.mak
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/bcc32.mak,v
retrieving revision 1.26
diff -c -r1.26 bcc32.mak
*** bcc32.mak 11 Jan 2007 02:42:31 -0000 1.26
--- bcc32.mak 8 Mar 2007 15:23:17 -0000
***************
*** 93,99 ****
-@erase "$(INTDIR)\fe-secure.obj"
-@erase "$(INTDIR)\pqexpbuffer.obj"
-@erase "$(INTDIR)\pqsignal.obj"
- -@erase "$(OUTDIR)\libpqdll.obj"
-@erase "$(OUTDIR)\win32.obj"
-@erase "$(INTDIR)\wchar.obj"
-@erase "$(INTDIR)\encnames.obj"
--- 93,98 ----
***************
*** 155,168 ****
LINK32=ilink32.exe
LINK32_FLAGS = -Gn -L$(BCB)\lib;$(INTDIR); -x -Tpd -v
- LINK32_OBJS= "$(INTDIR)\libpqdll.obj"
# @<< is a Response file, http://www.opussoftware.com/tutorial/TutMakefile.htm
! "$(OUTDIR)\blibpq.dll": "$(OUTDIR)\blibpq.lib" $(LINK32_OBJS) "$(INTDIR)\libpq.res" blibpqdll.def
$(LINK32) @<<
$(LINK32_FLAGS) +
! c0d32.obj $(LINK32_OBJS), +
$@,, +
"$(OUTDIR)\blibpq.lib" import32.lib cw32mt.lib, +
blibpqdll.def,"$(INTDIR)\libpq.res"
--- 154,166 ----
LINK32=ilink32.exe
LINK32_FLAGS = -Gn -L$(BCB)\lib;$(INTDIR); -x -Tpd -v
# @<< is a Response file, http://www.opussoftware.com/tutorial/TutMakefile.htm
! "$(OUTDIR)\blibpq.dll": "$(OUTDIR)\blibpq.lib" "$(INTDIR)\libpq.res" blibpqdll.def
$(LINK32) @<<
$(LINK32_FLAGS) +
! c0d32.obj , +
$@,, +
"$(OUTDIR)\blibpq.lib" import32.lib cw32mt.lib, +
blibpqdll.def,"$(INTDIR)\libpq.res"
Index: fe-connect.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.344
diff -c -r1.344 fe-connect.c
*** fe-connect.c 20 Feb 2007 15:20:51 -0000 1.344
--- fe-connect.c 8 Mar 2007 15:42:59 -0000
***************
*** 1840,1848 ****
#ifdef WIN32
/*
! * Make sure socket support is up and running. Even though this is done in
! * libpqdll.c, that is only for MSVC and BCC builds and doesn't work for
! * static builds at all, so we have to do it in the main code too.
*/
WSADATA wsaData;
--- 1840,1846 ----
#ifdef WIN32
/*
! * Make sure socket support is up and running.
*/
WSADATA wsaData;
***************
*** 1853,1859 ****
--- 1851,1862 ----
conn = (PGconn *) malloc(sizeof(PGconn));
if (conn == NULL)
+ {
+ #ifdef WIN32
+ WSACleanup();
+ #endif
return conn;
+ }
/* Zero all pointers and booleans */
MemSet(conn, 0, sizeof(PGconn));
***************
*** 1918,1927 ****
PGnotify *notify;
pgParameterStatus *pstatus;
- #ifdef WIN32
- WSACleanup();
- #endif
-
if (!conn)
return;
--- 1921,1926 ----
***************
*** 1986,1991 ****
--- 1985,1994 ----
termPQExpBuffer(&conn->errorMessage);
termPQExpBuffer(&conn->workBuffer);
free(conn);
+
+ #ifdef WIN32
+ WSACleanup();
+ #endif
}
/*
Index: win32.mak
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/win32.mak,v
retrieving revision 1.43
diff -c -r1.43 win32.mak
*** win32.mak 11 Jan 2007 02:42:31 -0000 1.43
--- win32.mak 8 Mar 2007 15:22:29 -0000
***************
*** 63,69 ****
-@erase "$(INTDIR)\fe-secure.obj"
-@erase "$(INTDIR)\pqexpbuffer.obj"
-@erase "$(INTDIR)\pqsignal.obj"
- -@erase "$(OUTDIR)\libpqdll.obj"
-@erase "$(OUTDIR)\win32.obj"
-@erase "$(INTDIR)\wchar.obj"
-@erase "$(INTDIR)\encnames.obj"
--- 63,68 ----
***************
*** 143,149 ****
/pdb:"$(OUTDIR)\libpqdll.pdb" /machine:I386 /out:"$(OUTDIR)\$(OUTFILENAME).dll"\
/implib:"$(OUTDIR)\$(OUTFILENAME)dll.lib" /def:$(OUTFILENAME)dll.def
LINK32_OBJS= \
- "$(INTDIR)\libpqdll.obj" \
"$(OUTDIR)\$(OUTFILENAME).lib" \
"$(OUTDIR)\libpq.res"
--- 142,147 ----
***************
*** 159,165 ****
$(RSC) $(RSC_PROJ) libpq.rc
! "$(OUTDIR)\$(OUTFILENAME).dll" : "$(OUTDIR)" "$(OUTDIR)\libpqdll.obj" "$(INTDIR)\libpqdll.obj" "$(INTDIR)\libpq.res"
$(LINK32) @<<
$(LINK32_FLAGS) $(LINK32_OBJS)
<<
--- 157,163 ----
$(RSC) $(RSC_PROJ) libpq.rc
! "$(OUTDIR)\$(OUTFILENAME).dll" : "$(OUTDIR)" "$(INTDIR)\libpq.res"
$(LINK32) @<<
$(LINK32_FLAGS) $(LINK32_OBJS)
<<
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