On Sun, Apr 6, 2014 at 11:45:59AM +0530, Amit Kapila wrote:
> On Tue, Apr 1, 2014 at 6:31 AM, Bruce Momjian <br...@momjian.us> wrote:
> > I reviewed this patch and you are correct that we are not handling
> > socket() and accept() returns properly on Windows. We were doing it
> > properly in most place in the backend, but your patch fixes the
> > remaining places:
> >
> >
> > http://msdn.microsoft.com/en-us/library/windows/desktop/ms740516%28v=vs.85%29.aspx
> >
> > However, libpq doesn't seem to be doing much to handle Windows properly
> > in this area. I have adjusted libpq to map socket to -1, but the proper
> > fix is to distribute pgsocket and PGINVALID_SOCKET checks throughout the
> > libpq code. I am not sure how to handle PQsocket() --- should it still
> > return -1?
>
> I think changing PQsocket() can impact all existing applications as
> it is mentioned in docs that "result of -1 indicates that no server
> connection is currently open.". Do you see any compelling need to
> change return value of PQSocket() after your patch?
No, I do not. In fact, the SSL_get_fd() call in secure_read() returns a
signed integer too, and that is passed around like a socket, so in fact
the SSL API doesn't even allow us to get an unsigned value on Windows in
all cases.
> > Having the return value be conditional on the operating
> > system is ugly. How much of this should be backpatched?
>
> I think it's okay to back patch all the changes.
> Is there any part in patch which you feel is risky to back patch?
Well, we would not backpatch this if it is just a stylistic fix, and I
am starting to think it just a style issue. This MSDN website says -1,
SOCKET_ERROR, and INVALID_SOCKET are very similar:
http://msdn.microsoft.com/en-us/library/windows/desktop/cc507522%28v=vs.85%29.aspx
and this Stackoverflow thread says the same:
http://stackoverflow.com/questions/10817252/why-is-invalid-socket-defined-as-0-in-winsock2-h-c
In fact, this C program compiled by gcc on Debian issues no compiler
warnings and returns 'hello', showing that -1 and ~0 compare as equal:
int
main(int argc, char **argv)
{
int i;
unsigned int j;
i = -1;
j = ~0;
if (i == j)
printf("hello\n");
return 0;
}
meaning our incorrect syntax is computed correctly.
> > Why aren't we
> > getting warnings on Windows about assigning the socket() return value to
> > an integer?
>
> I think by default Windows doesn't give warning for such code even at Warning
> level 4. I have found one related link:
> http://stackoverflow.com/questions/75385/make-vs-compiler-catch-signed-unsigned-assignments
>
> > Updated patch attached.
>
> It seems you have missed to change at below places.
>
> 1.
> int
> pg_foreach_ifaddr(PgIfAddrCallback callback, void *cb_data)
> sock = WSASocket(AF_INET, SOCK_DGRAM, 0, 0, 0, 0);
> if (sock == SOCKET_ERROR)
Well, the actual problem here is that WSASocket() returns INVALID_SOCKET
per the documentation, not SOCKET_ERROR. I did not use PGINVALID_SOCKET
here because this is Windows-specific code, defining 'sock' as SOCKET.
We could have sock defined as pgsocket, but because this is Windows code
already, it doesn't seem wise to mix portability code in there.
> 2.
> pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout)
> {
> static HANDLE waitevent = INVALID_HANDLE_VALUE;
> static SOCKET current_socket = -1;
Yes, that -1 is wrong and I have changed it to INVALID_SOCKET, again
using the same rules that say PGINVALID_SOCKET doesn't make sense here
as it is Windows-specific code.
I am attaching an updated patch, which explains the PQsocket() return
value issue, and fixes the items listed above. I am inclined to apply
this just to head for correctness, and modify libpq to use pgsocket
consistently in a follow-up patch.
This is not like the readdir() fix we had to backpatch because that was
clearly not catching errors.
--
Bruce Momjian <br...@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
new file mode 100644
index d062c1d..8fa9aa7
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
*************** ident_inet(hbaPort *port)
*** 1463,1469 ****
sock_fd = socket(ident_serv->ai_family, ident_serv->ai_socktype,
ident_serv->ai_protocol);
! if (sock_fd < 0)
{
ereport(LOG,
(errcode_for_socket_access(),
--- 1463,1469 ----
sock_fd = socket(ident_serv->ai_family, ident_serv->ai_socktype,
ident_serv->ai_protocol);
! if (sock_fd == PGINVALID_SOCKET)
{
ereport(LOG,
(errcode_for_socket_access(),
*************** ident_inet(hbaPort *port)
*** 1543,1549 ****
ident_response)));
ident_inet_done:
! if (sock_fd >= 0)
closesocket(sock_fd);
pg_freeaddrinfo_all(remote_addr.addr.ss_family, ident_serv);
pg_freeaddrinfo_all(local_addr.addr.ss_family, la);
--- 1543,1549 ----
ident_response)));
ident_inet_done:
! if (sock_fd != PGINVALID_SOCKET)
closesocket(sock_fd);
pg_freeaddrinfo_all(remote_addr.addr.ss_family, ident_serv);
pg_freeaddrinfo_all(local_addr.addr.ss_family, la);
*************** CheckRADIUSAuth(Port *port)
*** 2361,2367 ****
packet->length = htons(packet->length);
sock = socket(serveraddrs[0].ai_family, SOCK_DGRAM, 0);
! if (sock < 0)
{
ereport(LOG,
(errmsg("could not create RADIUS socket: %m")));
--- 2361,2367 ----
packet->length = htons(packet->length);
sock = socket(serveraddrs[0].ai_family, SOCK_DGRAM, 0);
! if (sock == PGINVALID_SOCKET)
{
ereport(LOG,
(errmsg("could not create RADIUS socket: %m")));
diff --git a/src/backend/libpq/ip.c b/src/backend/libpq/ip.c
new file mode 100644
index 7e8fc78..acdbab0
*** a/src/backend/libpq/ip.c
--- b/src/backend/libpq/ip.c
*************** pg_foreach_ifaddr(PgIfAddrCallback callb
*** 547,553 ****
int error;
sock = WSASocket(AF_INET, SOCK_DGRAM, 0, 0, 0, 0);
! if (sock == SOCKET_ERROR)
return -1;
while (n_ii < 1024)
--- 547,553 ----
int error;
sock = WSASocket(AF_INET, SOCK_DGRAM, 0, 0, 0, 0);
! if (sock == INVALID_SOCKET)
return -1;
while (n_ii < 1024)
*************** pg_foreach_ifaddr(PgIfAddrCallback callb
*** 670,676 ****
total;
sock = socket(AF_INET, SOCK_DGRAM, 0);
! if (sock == -1)
return -1;
while (n_buffer < 1024 * 100)
--- 670,676 ----
total;
sock = socket(AF_INET, SOCK_DGRAM, 0);
! if (sock == PGINVALID_SOCKET)
return -1;
while (n_buffer < 1024 * 100)
*************** pg_foreach_ifaddr(PgIfAddrCallback callb
*** 711,717 ****
#ifdef HAVE_IPV6
/* We'll need an IPv6 socket too for the SIOCGLIFNETMASK ioctls */
sock6 = socket(AF_INET6, SOCK_DGRAM, 0);
! if (sock6 == -1)
{
free(buffer);
close(sock);
--- 711,717 ----
#ifdef HAVE_IPV6
/* We'll need an IPv6 socket too for the SIOCGLIFNETMASK ioctls */
sock6 = socket(AF_INET6, SOCK_DGRAM, 0);
! if (sock6 == PGINVALID_SOCKET)
{
free(buffer);
close(sock);
*************** pg_foreach_ifaddr(PgIfAddrCallback callb
*** 788,797 ****
char *ptr,
*buffer = NULL;
size_t n_buffer = 1024;
! int sock;
sock = socket(AF_INET, SOCK_DGRAM, 0);
! if (sock == -1)
return -1;
while (n_buffer < 1024 * 100)
--- 788,797 ----
char *ptr,
*buffer = NULL;
size_t n_buffer = 1024;
! pgsocket sock;
sock = socket(AF_INET, SOCK_DGRAM, 0);
! if (sock == PGINVALID_SOCKET)
return -1;
while (n_buffer < 1024 * 100)
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
new file mode 100644
index 1eae183..0179451
*** a/src/backend/libpq/pqcomm.c
--- b/src/backend/libpq/pqcomm.c
*************** StreamServerPort(int family, char *hostN
*** 392,398 ****
break;
}
! if ((fd = socket(addr->ai_family, SOCK_STREAM, 0)) < 0)
{
ereport(LOG,
(errcode_for_socket_access(),
--- 392,398 ----
break;
}
! if ((fd = socket(addr->ai_family, SOCK_STREAM, 0)) == PGINVALID_SOCKET)
{
ereport(LOG,
(errcode_for_socket_access(),
*************** StreamConnection(pgsocket server_fd, Por
*** 632,638 ****
port->raddr.salen = sizeof(port->raddr.addr);
if ((port->sock = accept(server_fd,
(struct sockaddr *) & port->raddr.addr,
! &port->raddr.salen)) < 0)
{
ereport(LOG,
(errcode_for_socket_access(),
--- 632,638 ----
port->raddr.salen = sizeof(port->raddr.addr);
if ((port->sock = accept(server_fd,
(struct sockaddr *) & port->raddr.addr,
! &port->raddr.salen)) == PGINVALID_SOCKET)
{
ereport(LOG,
(errcode_for_socket_access(),
diff --git a/src/backend/port/win32/socket.c b/src/backend/port/win32/socket.c
new file mode 100644
index 4f1099f..adc0e02
*** a/src/backend/port/win32/socket.c
--- b/src/backend/port/win32/socket.c
*************** int
*** 132,138 ****
pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout)
{
static HANDLE waitevent = INVALID_HANDLE_VALUE;
! static SOCKET current_socket = -1;
static int isUDP = 0;
HANDLE events[2];
int r;
--- 132,138 ----
pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout)
{
static HANDLE waitevent = INVALID_HANDLE_VALUE;
! static SOCKET current_socket = INVALID_SOCKET;
static int isUDP = 0;
HANDLE events[2];
int r;
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
new file mode 100644
index bb771a6..b573fd8
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
*************** ConnCreate(int serverFd)
*** 2169,2175 ****
if (StreamConnection(serverFd, port) != STATUS_OK)
{
! if (port->sock >= 0)
StreamClose(port->sock);
ConnFree(port);
return NULL;
--- 2169,2175 ----
if (StreamConnection(serverFd, port) != STATUS_OK)
{
! if (port->sock != PGINVALID_SOCKET)
StreamClose(port->sock);
ConnFree(port);
return NULL;
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
new file mode 100644
index d53c41f..459e4a4
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*************** keep_going: /* We will come back to
*** 1632,1639 ****
conn->raddr.salen = addr_cur->ai_addrlen;
/* Open a socket */
! conn->sock = socket(addr_cur->ai_family, SOCK_STREAM, 0);
! if (conn->sock < 0)
{
/*
* ignore socket() failure if we have more addresses
--- 1632,1654 ----
conn->raddr.salen = addr_cur->ai_addrlen;
/* Open a socket */
! {
! /*
! * While we use 'pgsocket' as the socket type in the
! * backend, we use 'int' for libpq socket values.
! * This requires us to map PGINVALID_SOCKET to -1
! * on Windows.
! * See http://msdn.microsoft.com/en-us/library/windows/desktop/ms740516%28v=vs.85%29.aspx
! */
! pgsocket sock = socket(addr_cur->ai_family, SOCK_STREAM, 0);
! #ifdef WIN32
! if (sock == PGINVALID_SOCKET)
! conn->sock = -1;
! else
! #endif
! conn->sock = sock;
! }
! if (conn->sock == -1)
{
/*
* ignore socket() failure if we have more addresses
*************** internal_cancel(SockAddr *raddr, int be_
*** 3136,3142 ****
char *errbuf, int errbufsize)
{
int save_errno = SOCK_ERRNO;
! int tmpsock = -1;
char sebuf[256];
int maxlen;
struct
--- 3151,3157 ----
char *errbuf, int errbufsize)
{
int save_errno = SOCK_ERRNO;
! pgsocket tmpsock = PGINVALID_SOCKET;
char sebuf[256];
int maxlen;
struct
*************** internal_cancel(SockAddr *raddr, int be_
*** 3149,3155 ****
* We need to open a temporary connection to the postmaster. Do this with
* only kernel calls.
*/
! if ((tmpsock = socket(raddr->addr.ss_family, SOCK_STREAM, 0)) < 0)
{
strlcpy(errbuf, "PQcancel() -- socket() failed: ", errbufsize);
goto cancel_errReturn;
--- 3164,3170 ----
* We need to open a temporary connection to the postmaster. Do this with
* only kernel calls.
*/
! if ((tmpsock = socket(raddr->addr.ss_family, SOCK_STREAM, 0)) == PGINVALID_SOCKET)
{
strlcpy(errbuf, "PQcancel() -- socket() failed: ", errbufsize);
goto cancel_errReturn;
*************** cancel_errReturn:
*** 3220,3226 ****
maxlen);
strcat(errbuf, "\n");
}
! if (tmpsock >= 0)
closesocket(tmpsock);
SOCK_ERRNO_SET(save_errno);
return FALSE;
--- 3235,3241 ----
maxlen);
strcat(errbuf, "\n");
}
! if (tmpsock != PGINVALID_SOCKET)
closesocket(tmpsock);
SOCK_ERRNO_SET(save_errno);
return FALSE;
*************** PQerrorMessage(const PGconn *conn)
*** 5300,5308 ****
--- 5315,5333 ----
return conn->errorMessage.data;
}
+ /*
+ * In Windows, socket values are unsigned, and an invalid socket value
+ * (INVALID_SOCKET) is ~0, which equals -1 in comparisons (with no compiler
+ * warning). Ideally we would return an unsigned value for PQsocket() on
+ * Windows, but that would cause the function's return value to differ from
+ * Unix, so we just return -1 for invalid sockets.
+ * http://msdn.microsoft.com/en-us/library/windows/desktop/cc507522%28v=vs.85%29.aspx
+ * http://stackoverflow.com/questions/10817252/why-is-invalid-socket-defined-as-0-in-winsock2-h-c
+ */
int
PQsocket(const PGconn *conn)
{
+
if (!conn)
return -1;
return conn->sock;
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
new file mode 100644
index 22bbe4a..ee975d4
*** a/src/interfaces/libpq/libpq-int.h
--- b/src/interfaces/libpq/libpq-int.h
*************** struct pg_conn
*** 364,369 ****
--- 364,370 ----
PGnotify *notifyTail; /* newest unreported Notify msg */
/* Connection data */
+ /* See PQconnectPoll() for how we use 'int' and not 'pgsocket'. */
int sock; /* Unix FD for socket, -1 if not connected */
SockAddr laddr; /* Local address */
SockAddr raddr; /* Remote address */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
