Обсуждение: [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of < 0

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.

Joel Jacobson

On Wed, Dec 25, 2013 at 6:05 PM, Joel Jacobson <joel@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

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

On Tue, Apr 1, 2014 at 6:31 AM, Bruce Momjian <bruce@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

On Sun, Apr  6, 2014 at 11:45:59AM +0530, Amit Kapila wrote:
> On Tue, Apr 1, 2014 at 6:31 AM, Bruce Momjian <bruce@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  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

On Tue, Apr 8, 2014 at 11:32 PM, Bruce Momjian <bruce@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

On Thu, Apr 10, 2014 at 11:05:49AM +0530, Amit Kapila wrote:
> On Tue, Apr 8, 2014 at 11:32 PM, Bruce Momjian <bruce@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  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Thu, Apr 10, 2014 at 5:21 PM, Bruce Momjian <bruce@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

On Fri, Apr 11, 2014 at 10:00 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Apr 10, 2014 at 5:21 PM, Bruce Momjian <bruce@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

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.kapila16@gmail.com> wrote:
> > On Thu, Apr 10, 2014 at 5:21 PM, Bruce Momjian <bruce@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  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

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.kapila16@gmail.com> wrote:
> > > On Thu, Apr 10, 2014 at 5:21 PM, Bruce Momjian <bruce@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  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

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  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

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  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +