Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0

2014-04-16 Thread Alvaro Herrera
Bruce Momjian wrote:
 
 Yes, I saw that yesterday and fixed it.  I also did a dry run of
 backpatching and only 8.4 had conflicts, so I think we are good there.
 (This is like the readdir() fix all over again.)
 
 Once this is applied I will work on changing the libpq socket type to
 use portable pgsocket, but I am not planning to backpatch that unless we
 find a bug.

Are we done with this patch?

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0

2014-04-16 Thread Bruce Momjian
On Wed, Apr 16, 2014 at 10:34:55AM -0300, Alvaro Herrera wrote:
 Bruce Momjian wrote:
  
  Yes, I saw that yesterday and fixed it.  I also did a dry run of
  backpatching and only 8.4 had conflicts, so I think we are good there.
  (This is like the readdir() fix all over again.)
  
  Once this is applied I will work on changing the libpq socket type to
  use portable pgsocket, but I am not planning to backpatch that unless we
  find a bug.
 
 Are we done with this patch?

I am about to patch it now.   I was going to do it yesterday but was
concerned I wouldn't be online long enough to catch any buildfarm
failure.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0

2014-04-16 Thread Bruce Momjian
On Fri, Apr 11, 2014 at 08:28:55AM -0400, Bruce Momjian wrote:
 On Fri, Apr 11, 2014 at 10:03:08AM +0530, Amit Kapila wrote:
  On Fri, Apr 11, 2014 at 10:00 AM, Amit Kapila amit.kapil...@gmail.com 
  wrote:
   On Thu, Apr 10, 2014 at 5:21 PM, Bruce Momjian br...@momjian.us wrote:
   On Thu, Apr 10, 2014 at 11:05:49AM +0530, Amit Kapila wrote:
  
   Ah, yes, good point.  This is going to require backpatching then.
  
   I also think so.
  
   I think it's better to use check like below, just for matter of
   consistency with other place
   if (sock == INVALID_SOCKET)
  
   Agreed.  That is how I have coded the patch.
  
   Sorry, I didn't checked the latest patch before that comment.
  
   I verified that your last patch is fine.  Regression test also went fine.
  
  I have noticed small thing which I forgot to mention in previous mail.
  I think below added extra line is not required.
  
int
PQsocket(const PGconn *conn)
{
  +
 
 Yes, I saw that yesterday and fixed it.  I also did a dry run of
 backpatching and only 8.4 had conflicts, so I think we are good there.
 (This is like the readdir() fix all over again.)

Patch applied back through 9.0.  8.4 didn't have the infrastructure for
a proper fix.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0

2014-04-16 Thread Bruce Momjian
On Fri, Apr 11, 2014 at 08:28:55AM -0400, Bruce Momjian wrote:
 Once this is applied I will work on changing the libpq socket type to
 use portable pgsocket, but I am not planning to backpatch that unless we
 find a bug.

Attached is a follow up patch which stores socket values in libpq as
pgsocket, rather than int, and maps it to -1 only for the PQsocket()
external return value.  In the interest of time, I will apply this later
today, and only to head as it does not fix a bug.

This is the last open item I was working on.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
new file mode 100644
index 51d4de4..90b944a
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*** pqDropConnection(PGconn *conn)
*** 398,406 
  	/* Drop any SSL state */
  	pqsecure_close(conn);
  	/* Close the socket itself */
! 	if (conn-sock = 0)
  		closesocket(conn-sock);
! 	conn-sock = -1;
  	/* Discard any unread/unsent data */
  	conn-inStart = conn-inCursor = conn-inEnd = 0;
  	conn-outCount = 0;
--- 398,406 
  	/* Drop any SSL state */
  	pqsecure_close(conn);
  	/* Close the socket itself */
! 	if (conn-sock != PGINVALID_SOCKET)
  		closesocket(conn-sock);
! 	conn-sock = PGINVALID_SOCKET;
  	/* Discard any unread/unsent data */
  	conn-inStart = conn-inCursor = conn-inEnd = 0;
  	conn-outCount = 0;
*** keep_going:		/* We will come back to
*** 1631,1654 
  		   addr_cur-ai_addrlen);
  	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
--- 1631,1638 
  		   addr_cur-ai_addrlen);
  	conn-raddr.salen = addr_cur-ai_addrlen;
  
! 	conn-sock = socket(addr_cur-ai_family, SOCK_STREAM, 0);
! 	if (conn-sock == PGINVALID_SOCKET)
  	{
  		/*
  		 * ignore socket() failure if we have more addresses
*** makeEmptyPGconn(void)
*** 2717,2723 
  	conn-client_encoding = PG_SQL_ASCII;
  	conn-std_strings = false;	/* unless server says differently */
  	conn-verbosity = PQERRORS_DEFAULT;
! 	conn-sock = -1;
  	conn-auth_req_received = false;
  	conn-password_needed = false;
  	conn-dot_pgpass_used = false;
--- 2701,2707 
  	conn-client_encoding = PG_SQL_ASCII;
  	conn-std_strings = false;	/* unless server says differently */
  	conn-verbosity = PQERRORS_DEFAULT;
! 	conn-sock = PGINVALID_SOCKET;
  	conn-auth_req_received = false;
  	conn-password_needed = false;
  	conn-dot_pgpass_used = false;
*** closePGconn(PGconn *conn)
*** 2882,2888 
  	 * Note that the protocol doesn't allow us to send Terminate messages
  	 * during the startup phase.
  	 */
! 	if (conn-sock = 0  conn-status == CONNECTION_OK)
  	{
  		/*
  		 * Try to send close connection message to backend. Ignore any
--- 2866,2872 
  	 * Note that the protocol doesn't allow us to send Terminate messages
  	 * during the startup phase.
  	 */
! 	if (conn-sock != PGINVALID_SOCKET  conn-status == CONNECTION_OK)
  	{
  		/*
  		 * Try to send close connection message to backend. Ignore any
*** PQgetCancel(PGconn *conn)
*** 3103,3109 
  	if (!conn)
  		return NULL;
  
! 	if (conn-sock  0)
  		return NULL;
  
  	cancel = malloc(sizeof(PGcancel));
--- 3087,3093 
  	if (!conn)
  		return NULL;
  
! 	if (conn-sock == PGINVALID_SOCKET)
  		return NULL;
  
  	cancel = malloc(sizeof(PGcancel));
*** PQrequestCancel(PGconn *conn)
*** 3284,3290 
  	if (!conn)
  		return FALSE;
  
! 	if (conn-sock  0)
  	{
  		strlcpy(conn-errorMessage.data,
  PQrequestCancel() -- connection is not open\n,
--- 3268,3274 
  	if (!conn)
  		return FALSE;
  
! 	if (conn-sock == PGINVALID_SOCKET)
  	{
  		strlcpy(conn-errorMessage.data,
  PQrequestCancel() -- connection is not open\n,
*** PQsocket(const PGconn *conn)
*** 5329,5335 
  {
  	if (!conn)
  		return -1;
! 	return conn-sock;
  }
  
  int
--- 5313,5319 
  {
  	if (!conn)
  		return -1;
! 	return (conn-sock != PGINVALID_SOCKET) ? conn-sock : -1;
  }
  
  int
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
new file mode 100644
index 8ccf6d3..50e4035
*** 

Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0

2014-04-16 Thread Bruce Momjian
On Wed, Apr 16, 2014 at 11:22:28AM -0400, Bruce Momjian wrote:
 On Fri, Apr 11, 2014 at 08:28:55AM -0400, Bruce Momjian wrote:
  Once this is applied I will work on changing the libpq socket type to
  use portable pgsocket, but I am not planning to backpatch that unless we
  find a bug.
 
 Attached is a follow up patch which stores socket values in libpq as
 pgsocket, rather than int, and maps it to -1 only for the PQsocket()
 external return value.  In the interest of time, I will apply this later
 today, and only to head as it does not fix a bug.
 
 This is the last open item I was working on.

Applied.  FYI, Joel Jacobson was the initial author of this thread of
patches;  report by PVS-Studio.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0

2014-04-11 Thread Bruce Momjian
On Fri, Apr 11, 2014 at 10:03:08AM +0530, Amit Kapila wrote:
 On Fri, Apr 11, 2014 at 10:00 AM, Amit Kapila amit.kapil...@gmail.com wrote:
  On Thu, Apr 10, 2014 at 5:21 PM, Bruce Momjian br...@momjian.us wrote:
  On Thu, Apr 10, 2014 at 11:05:49AM +0530, Amit Kapila wrote:
 
  Ah, yes, good point.  This is going to require backpatching then.
 
  I also think so.
 
  I think it's better to use check like below, just for matter of
  consistency with other place
  if (sock == INVALID_SOCKET)
 
  Agreed.  That is how I have coded the patch.
 
  Sorry, I didn't checked the latest patch before that comment.
 
  I verified that your last patch is fine.  Regression test also went fine.
 
 I have noticed small thing which I forgot to mention in previous mail.
 I think below added extra line is not required.
 
   int
   PQsocket(const PGconn *conn)
   {
 +

Yes, I saw that yesterday and fixed it.  I also did a dry run of
backpatching and only 8.4 had conflicts, so I think we are good there.
(This is like the readdir() fix all over again.)

Once this is applied I will work on changing the libpq socket type to
use portable pgsocket, but I am not planning to backpatch that unless we
find a bug.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0

2014-04-10 Thread Bruce Momjian
On Thu, Apr 10, 2014 at 11:05:49AM +0530, Amit Kapila wrote:
 On Tue, Apr 8, 2014 at 11:32 PM, Bruce Momjian br...@momjian.us wrote:
  On Sun, Apr  6, 2014 at 11:45:59AM +0530, Amit Kapila wrote:
  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;
  }
 
 I have add below code to check it's usage as per PG:
 
 if (j  0)
 printf(hello-1\n);
 
 It doesn't print hello-1, which means that all the check's in code
 for sock_desc  0 can have problem.

Ah, yes, good point.  This is going to require backpatching then.

  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.
 
 I think it's better to use check like below, just for matter of
 consistency with other place
 if (sock == INVALID_SOCKET)

Agreed.  That is how I have coded the patch.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0

2014-04-10 Thread Amit Kapila
On Thu, Apr 10, 2014 at 5:21 PM, Bruce Momjian br...@momjian.us wrote:
 On Thu, Apr 10, 2014 at 11:05:49AM +0530, Amit Kapila wrote:

 Ah, yes, good point.  This is going to require backpatching then.

I also think so.

 I think it's better to use check like below, just for matter of
 consistency with other place
 if (sock == INVALID_SOCKET)

 Agreed.  That is how I have coded the patch.

Sorry, I didn't checked the latest patch before that comment.

I verified that your last patch is fine.  Regression test also went fine.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0

2014-04-10 Thread Amit Kapila
On Fri, Apr 11, 2014 at 10:00 AM, Amit Kapila amit.kapil...@gmail.com wrote:
 On Thu, Apr 10, 2014 at 5:21 PM, Bruce Momjian br...@momjian.us wrote:
 On Thu, Apr 10, 2014 at 11:05:49AM +0530, Amit Kapila wrote:

 Ah, yes, good point.  This is going to require backpatching then.

 I also think so.

 I think it's better to use check like below, just for matter of
 consistency with other place
 if (sock == INVALID_SOCKET)

 Agreed.  That is how I have coded the patch.

 Sorry, I didn't checked the latest patch before that comment.

 I verified that your last patch is fine.  Regression test also went fine.

I have noticed small thing which I forgot to mention in previous mail.
I think below added extra line is not required.

  int
  PQsocket(const PGconn *conn)
  {
+

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0

2014-04-09 Thread Amit Kapila
On Tue, Apr 8, 2014 at 11:32 PM, Bruce Momjian br...@momjian.us wrote:
 On Sun, Apr  6, 2014 at 11:45:59AM +0530, Amit Kapila wrote:
 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;
 }

I have add below code to check it's usage as per PG:

if (j  0)
printf(hello-1\n);

It doesn't print hello-1, which means that all the check's in code
for sock_desc  0 can have problem.

 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.

I think it's better to use check like below, just for matter of
consistency with other place
if (sock == INVALID_SOCKET)

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0

2014-04-08 Thread Bruce Momjian
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.ushttp://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);
  	

Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0

2014-04-06 Thread Amit Kapila
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?

 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?

  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)

2.
pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout)
{
static HANDLE waitevent = INVALID_HANDLE_VALUE;
static SOCKET current_socket = -1;

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0

2014-03-31 Thread Bruce Momjian
On Wed, Dec 25, 2013 at 01:35:15PM +0100, Joel Jacobson wrote:
 Hi,
 
 I've tried to fix some bugs reported by Andrey Karpov in an article at
 http://www.viva64.com/en/b/0227/
 
 The value returned by socket() is unsigned on Windows and can thus not
 be checked if less than zero to detect an error, instead
 PGINVALID_SOCKET should be used, which is hard-coded to -1 on
 non-windows platforms.

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?  Having the return value be conditional on the operating
system is ugly.  How much of this should be backpatched?  Why aren't we
getting warnings on Windows about assigning the socket() return value to
an integer?

Updated patch attached.

-- 
  Bruce Momjian  br...@momjian.ushttp://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 31ade0b..f674372
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
*** ident_inet(hbaPort *port)
*** 1453,1459 
  
  	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(),
--- 1453,1459 
  
  	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)
*** 1533,1539 
  	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);
--- 1533,1539 
  	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)
*** 2351,2357 
  	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)));
--- 2351,2357 
  	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..4db64fb
*** a/src/backend/libpq/ip.c
--- b/src/backend/libpq/ip.c
*** 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;
  		}
  

Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0

2014-03-05 Thread Alvaro Herrera
Joel Jacobson wrote:
 Hi,
 
 I've tried to fix some bugs reported by Andrey Karpov in an article at
 http://www.viva64.com/en/b/0227/
 
 The value returned by socket() is unsigned on Windows and can thus not
 be checked if less than zero to detect an error, instead
 PGINVALID_SOCKET should be used, which is hard-coded to -1 on
 non-windows platforms.

Can I bug you into verifying what supported releases need this patch,
and to which does it backpatch cleanly?  And if there's any to which it
doesn't, can I further bug you into providing one that does?

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0

2013-12-31 Thread Amit Kapila
On Wed, Dec 25, 2013 at 6:05 PM, Joel Jacobson j...@trustly.com wrote:
 Hi,

 I've tried to fix some bugs reported by Andrey Karpov in an article at
 http://www.viva64.com/en/b/0227/

 The value returned by socket() is unsigned on Windows and can thus not
 be checked if less than zero to detect an error, instead
 PGINVALID_SOCKET should be used, which is hard-coded to -1 on
 non-windows platforms.

Though I have not verified all instances you have fixed in your patch, but I
feel the general direction to fix is right.
I think you can upload your patch for next CommitFest.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers