From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49614) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1anxNY-0001pt-Ul for qemu-devel@nongnu.org; Wed, 06 Apr 2016 20:00:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1anxNW-0000o8-Ce for qemu-devel@nongnu.org; Wed, 06 Apr 2016 20:00:56 -0400 Received: from nm27-vm5.bullet.mail.ne1.yahoo.com ([98.138.91.249]:46406) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1anxNW-0000nx-6q for qemu-devel@nongnu.org; Wed, 06 Apr 2016 20:00:54 -0400 Date: Wed, 6 Apr 2016 17:00:50 -0700 From: Steven Luo Message-ID: <20160407000050.GA21369@steven676.net> References: <1c0611a61621b2069df831b48b937fa49cc264bd.1459896208.git.steven@steven676.net> <20160406125743.GC28379@var.bordeaux.inria.fr> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160406125743.GC28379@var.bordeaux.inria.fr> Subject: Re: [Qemu-devel] [PATCH 0/3] slirp: deliver received TCP RSTs to the guest List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Samuel Thibault Cc: "Edgar E. Iglesias" , Jan Kiszka , qemu-devel@nongnu.org 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