Skip to content

Fix vc close migration race condition #7337

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shinrich merged 2 commits into from
Nov 23, 2020
Merged

Fix vc close migration race condition #7337

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c5779f
Fix vc close migration race condition
shinrich Nov 20, 2020
f34e8ec
Delay the vio cont clearing
shinrich Nov 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions iocore/net/UnixNetVConnection.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,17 +652,11 @@ UnixNetVConnection::do_io_close(int alerrno /* = -1 */)
// FIXME: the nh must not nullptr.
ink_assert(nh);

// mark it closed first
if (alerrno == -1) {
closed = 1;
} else {
closed = -1;
}
// The vio continuations will be cleared in ::clear called from ::free
read.enabled = 0;
write.enabled = 0;
read.vio.nbytes = 0;
read.vio.op = VIO::NONE;
read.vio.cont = nullptr;

if (netvc_context == NET_VCONNECTION_OUT) {
// do not clear the iobufs yet to guard
Expand All @@ -676,7 +670,6 @@ UnixNetVConnection::do_io_close(int alerrno /* = -1 */)

write.vio.nbytes = 0;
write.vio.op = VIO::NONE;
write.vio.cont = nullptr;

EThread *t = this_ethread();
bool close_inline = !recursion && (!nh || nh->mutex->thread_holding == t);
Expand All @@ -686,6 +679,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) {
Copy link
Contributor

@sudheerv sudheerv Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@shinrich shinrich Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try moving clearing the vio contintations to the free. But even reducing the race window this tiny amount seems to have eliminated the crash for us.

closed = 1;
} else {
closed = -1;
}

if (close_inline) {
if (nh) {
nh->free_netevent(this);
Expand Down