Copilot commented on code in PR #13821:
URL: https://github.com/apache/trafficserver/pull/13821#discussion_r4253053036
##########
src/iocore/net/UnixNetVConnection.cc:
##########
@@ -269,22 +271,33 @@ UnixNetVConnection::do_io_close(int alerrno /* = -1 */)
write.vio.nbytes = 0;
write.vio.op = VIO::NONE;
- EThread *t = this_ethread();
- bool close_inline = !recursion && (!nh || nh->mutex->thread_holding ==
t);
+ EThread *t = this_ethread();
+ bool close_inline = !recursion && (!nh || nh->mutex->thread_holding
== t);
+ bool const on_owner = (t == this->thread);
+ NetHandler *owner_nh = nh;
INK_WRITE_MEMORY_BARRIER;
if (alerrno && alerrno != -1) {
this->lerrno = alerrno;
}
// Must mark for closed last in case this is a
- // cross thread migration scenario.
+ // cross thread migration scenario. Once it is set, a foreign close must not
+ // touch this NetEvent again, since its owning thread may free it at any
time.
if (alerrno == -1) {
closed = 1;
} else {
closed = -1;
}
+ // Only the owning thread may touch the timer wheel. A foreign close that is
+ // not freeing inline hands the NetEvent back so the owner can free it.
+ if (on_owner) {
+ this->rearm_timer();
+ } else if (owner_nh && !close_inline) {
+ owner_nh->signal_closed(this);
Review Comment:
There is a use-after-free window here: after `closed` is published, the
owner thread may observe it in `process_ready_list()` and free the NetEvent
before this foreign thread enters `signal_closed()`, which then accesses
`read.in_enabled_list`. The handoff needs a lifetime/ownership protocol that is
established before publishing the close and guarantees the object remains alive
until the owner consumes it.
##########
src/iocore/net/UnixNetVConnection.cc:
##########
@@ -455,6 +468,15 @@ UnixNetVConnection::set_enabled(VIO *vio)
STATE_FROM_VIO(vio)->enabled = 1;
if (!next_inactivity_timeout_at && inactivity_timeout_in) {
next_inactivity_timeout_at = ink_get_hrtime() + inactivity_timeout_in;
+ rearm_timer();
Review Comment:
`reenable()` calls `set_enabled()` before it determines whether the caller
owns the NetHandler mutex; its existing fallback at
`UnixNetVConnection.cc:383-400` explicitly supports a foreign thread by
queueing on `read_enable_list`/`write_enable_list`. This new call therefore
mutates the owner thread's non-thread-safe timer wheel from the foreign thread
and can corrupt its intrusive buckets. Defer the rearm through the existing
owner-thread handoff (and perform it under the NetHandler mutex).
This issue also appears on line 479 of the same file.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
