From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B53E246970F for ; Tue, 26 Nov 2019 10:51:23 +0300 (MSK) Date: Tue, 26 Nov 2019 10:51:22 +0300 From: Kirill Yukhin Message-ID: <20191126075122.zq5ysplxtcvvdrhh@tarantool.org> References: <4c563cef125a2b96a3379defc40fdae4c2a0bc6d.1574112599.git.v.shpilevoy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4c563cef125a2b96a3379defc40fdae4c2a0bc6d.1574112599.git.v.shpilevoy@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v3 1/1] iproto: don't destroy a session during disconnect List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Hello, On 18 ноя 22:31, Vladislav Shpilevoy wrote: > Binary session disconnect trigger yield could lead to use after > free of the session object. That happened because iproto thread > sent two requests to TX thread at disconnect: > > - Close the session and run its on disconnect triggers; > > - If all requests are handled, destroy the session. > > When a connection is idle, all requests are handled, so both these > requests are sent. If the first one yielded in TX thread, the > second one arrived and destroyed the session right under the feet > of the first one. > > This can be solved in two ways - in TX thread, and in iproto > thread. > > Iproto thread solution (which is chosen in the patch): just don't > send destroy request until disconnect returns back to iproto > thread. > > TX thread solution (alternative): add a flag which says whether > disconnect is processed by TX. When destroy request arrives, it > checks the flag. If disconnect is not done, the destroy request > waits on a condition variable until it is. > > The iproto is a bit tricker to implement, but it looks more > correct. > > Closes #4627 I've checked your patch into 1.10, 2.2 and master. -- Regards, Kirill Yukhin