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

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. +