[GitHub] [trafficserver] sudheerv commented on a change in pull request #7337: Fix vc close migration race condition

[GitBox <gi...@apache.org>]

sudheerv commented on a change in pull request #7337:
URL: https://github.com/apache/trafficserver/pull/7337#discussion_r527799494



##########
File path: iocore/net/UnixNetVConnection.cc
##########
@@ -686,6 +680,14 @@ UnixNetVConnection::do_io_close(int alerrno /* = -1 */)
     this->lerrno = alerrno;
   }
 
+  // Must mark for closed last in case this is a
+  // cross thread migration scenario.
+  if (alerrno == -1) {

Review comment:
       Does setting `closed` here really prevent the use-after-free? Presumably, this thread is still trying to clean up the VC, which means it's still running the ::free/::clear routines. Wouldn't the race still exist for the read ready loop then? I wonder you will still see the issue from ASAN even with this new change.
   
   I think we do need to delay setting the read.vio.cont and write.vio.cont to nullptr in do_io_close to after setting close  flag, as otherwise, the net_read_io may get unblocked on the read vio mutex and run a previously rescheduled read event




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org