[erlang-bugs] : : : : : Follow up: [BUG] gen_tcp:connect/3, 4 returns socket for closed port

[Raimo Niskanen]

On Mon, Sep 15, 2008 at 11:17:42AM -0400, Edwin Fine wrote:
> Raimo,
> 
> Great work! You found it pretty quickly. I'll apply the patch as soon as
> possible.
> I'll wait for a while before applying the patch to production systems until
> the regression tests are done.

We found no problems with this patch in the regression tests.
Now we will see what you and the other users find...



> 
> Thanks very much.
> Regards,
> Edwin Fine
> 
> On Mon, Sep 15, 2008 at 10:54 AM, Raimo Niskanen <
> raimo+erlang-bugs@REDACTED <raimo%2Berlang-bugs@REDACTED>>wrote:
> 
:
> > Found the bug!
> >
> > It was a clear and simple bug in the TCP|UDP|SCTP/IP driver
> > inet_drv, where making a connect, the poll set was changed
> > first and then connect was called, and if connect did
> > not give EINPROGRESS, the poll set was reset again.
> >
> > This is the wrong way to do it. The right is to change
> > the poll set if you have to after getting EINPROGRESS.
> >
> > What happened now was:
> > [thread 1]                      [thread 2]
> >
> >                                poll( ...
> > socket() -> Socket
> > Change pollset data
> > write(InternalPipe)
> >  to inform poll thread
> >                                ...)poll -> internal pipe POLLIN|POLLRDNORM
> >                                read(InternalPipe)
> > connect(Socket, ...
> >                                poll() -> Socket POLLOUT|POLLHUP
> > ... )connect -> EINPROGRESS
> >                                getsockopt(Socket, SOL_SOCKET, SO_ERROR) ->
> >                                  no error
> >
> > Note that poll returns with ready for writing on Socket
> > before connect returns with EINPROGRESS in the other
> > thread, and Linus only knows why getsockopt() in this
> > particular case returns no error.
> >
> > But we are mishandling the socket, that is for sure.
> > Try this patch:
> > *** /clearcase/otp/erts/erts/emulator/drivers/common/inet_drv.c@@/OTP_R12B-4
> >    2008-09-01 14:51:18.000000000 +0200
> > --- /clearcase/otp/erts/erts/emulator/drivers/common/inet_drv.c 2008-09-15
> > 16:23:51.000000000 +0200
> > ***************
> > *** 7239,7257 ****
> >                             buf, &len) == NULL)
> >            return ctl_error(EINVAL, rbuf, rsize);
> >
> > -       sock_select(INETP(desc), FD_CONNECT, 1);
> >        code = sock_connect(desc->inet.s,
> >                            (struct sockaddr*) &desc->inet.remote, len);
> >        if ((code == SOCKET_ERROR) &&
> >                ((sock_errno() == ERRNO_BLOCK) ||  /* Winsock2 */
> >                 (sock_errno() == EINPROGRESS))) {      /* Unix & OSE!! */
> >            desc->inet.state = TCP_STATE_CONNECTING;
> >            if (timeout != INET_INFINITY)
> >                driver_set_timer(desc->inet.port, timeout);
> >            enq_async(INETP(desc), tbuf, INET_REQ_CONNECT);
> >        }
> >        else if (code == 0) { /* ok we are connected */
> > -           sock_select(INETP(desc), FD_CONNECT, 0);
> >            desc->inet.state = TCP_STATE_CONNECTED;
> >            if (desc->inet.active)
> >                sock_select(INETP(desc), (FD_READ|FD_CLOSE), 1);
> > --- 7239,7256 ----
> >                             buf, &len) == NULL)
> >            return ctl_error(EINVAL, rbuf, rsize);
> >
> >        code = sock_connect(desc->inet.s,
> >                            (struct sockaddr*) &desc->inet.remote, len);
> >        if ((code == SOCKET_ERROR) &&
> >                ((sock_errno() == ERRNO_BLOCK) ||  /* Winsock2 */
> >                 (sock_errno() == EINPROGRESS))) {      /* Unix & OSE!! */
> > +           sock_select(INETP(desc), FD_CONNECT, 1);
> >            desc->inet.state = TCP_STATE_CONNECTING;
> >            if (timeout != INET_INFINITY)
> >                driver_set_timer(desc->inet.port, timeout);
> >            enq_async(INETP(desc), tbuf, INET_REQ_CONNECT);
> >        }
> >        else if (code == 0) { /* ok we are connected */
> >            desc->inet.state = TCP_STATE_CONNECTED;
> >            if (desc->inet.active)
> >                sock_select(INETP(desc), (FD_READ|FD_CLOSE), 1);
> > ***************
> > *** 7259,7265 ****
> >            async_ok(INETP(desc));
> >        }
> >        else {
> > -           sock_select(INETP(desc), FD_CONNECT, 0);
> >            return ctl_error(sock_errno(), rbuf, rsize);
> >        }
> >        return ctl_reply(INET_REP_OK, tbuf, 2, rbuf, rsize);
> > --- 7258,7263 ----
> >
> > The patch has not been run through all our regression tests yet.
> >
> > --
> >
> > / Raimo Niskanen, Erlang/OTP, Ericsson AB
> >
> >

-- 

/ Raimo Niskanen, Erlang/OTP, Ericsson AB