[0/3] slirp: deliver received TCP RSTs to the guest

Hello,

Thanks for this!

steven@steven676.net, on Tue 05 Apr 2016 17:13:58 -0700, wrote:
> The second,
> which fixes delivery of an RST interrupting an already-established TCP
> connection, was submitted by Edgar Iglesias in 2008 and appears to have
> been missed then.  The last patch fixes the case where the remote end
> sends RST in reply to our SYN (rejects our incoming connection attempt).

It seems I'm getting another crash with these: sowrite would be called
too for the reseted socket, while the socket has been freed and is not
even on the polling list any more, I had to additionally do the patch
below, could you review it so I can push the whole series?

> I've been unable to test the specific case it
> addresses (an established connection interrupted by RST),

It's actually quite easy: just reboot the server :) The new instance of
the server will send a RST whenever the client sends more data.

Samuel

On Wed, Apr 06, 2016 at 02:57:43PM +0200, Samuel Thibault wrote:
> steven@steven676.net, on Tue 05 Apr 2016 17:13:58 -0700, wrote:
> > The second,
> > which fixes delivery of an RST interrupting an already-established TCP
> > connection, was submitted by Edgar Iglesias in 2008 and appears to have
> > been missed then.  The last patch fixes the case where the remote end
> > sends RST in reply to our SYN (rejects our incoming connection attempt).
> 
> It seems I'm getting another crash with these: sowrite would be called
> too for the reseted socket, while the socket has been freed and is not
> even on the polling list any more, I had to additionally do the patch
> below, could you review it so I can push the whole series?

I can't reproduce the crash, but the !(so->so_state & SS_NOFDREF) test
immediately below would seem to be a use-after-free in this case, so I
figure we do need something like this.  That said, sorecvoob() also
calls soread(), so I'd guess we need to deal with the possibility that
soread() frees the socket in that case as well?  (I can't find any other
callers of soread(), but if they exist, they probably need to be fixed
too.)

I could take care of this when I resend this patch series, if you
prefer.

> It's actually quite easy: just reboot the server :) The new instance of
> the server will send a RST whenever the client sends more data.

Thanks for the hint -- I've verified that case works as well now.

> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index fef526c..b13b9af 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -553,6 +553,11 @@ void slirp_pollfds_poll(GArray *pollfds, int select_error)
>                      if (ret > 0) {
>                          tcp_output(sototcpcb(so));
>                      }
> +                    if (ret < 0) {
> +                        /* Socket error and thus removed, do not try to do
> +                         * anything more with it.  */

I think this should be "might have been removed"?  tcp_sockclosed()
doesn't seem to call tcp_close() in every case, so we can get -1 from
soread() without the socket being freed.

> +                        continue;
> +                    }
>                  }
>  
>                  /*

-Steven Luo

Hello,

Steven Luo, on Wed 06 Apr 2016 17:00:50 -0700, wrote:
> That said, sorecvoob() also calls soread(), so I'd guess we need to
> deal with the possibility that soread() frees the socket in that case
> as well?

Indeed, then sorecvoob() needs to return that information, so
slirp_pollfds_poll can avoid doing anything else with this socket.

> I could take care of this when I resend this patch series, if you
> prefer.

If you like, please do :)

> I think this should be "might have been removed"?  tcp_sockclosed()
> doesn't seem to call tcp_close() in every case, so we can get -1 from
> soread() without the socket being freed.

Right.

Samuel