Skip to content

Commit 074671f

Browse files
authored
Revert "Fix NetAcceptAction::cancel() use-after-free race condition (#12803)" (#12841)
This reverts commit 89a5def. I suspect this is introducing different crashes in NetAccept::cancel in CI regression tests.
1 parent 15cdb35 commit 074671f

4 files changed

Lines changed: 16 additions & 24 deletions

File tree

src/iocore/net/P_NetAccept.h

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@
4343
#include "iocore/net/NetAcceptEventIO.h"
4444
#include "Server.h"
4545

46-
#include <atomic>
4746
#include <vector>
4847

4948
struct NetAccept;
@@ -61,29 +60,21 @@ AcceptFunction net_accept;
6160

6261
class UnixNetVConnection;
6362

63+
// TODO fix race between cancel accept and call back
6464
struct NetAcceptAction : public Action, public RefCountObjInHeap {
65-
std::atomic<Server *> server{nullptr};
66-
67-
NetAcceptAction(Continuation *cont, Server *s)
68-
{
69-
continuation = cont;
70-
if (cont != nullptr) {
71-
mutex = cont->mutex;
72-
}
73-
server.store(s, std::memory_order_release);
74-
}
65+
Server *server;
7566

7667
void
7768
cancel(Continuation *cont = nullptr) override
7869
{
79-
// Close the server before setting the cancelled flag. This ensures that
80-
// when acceptEvent() sees cancelled == true, the server close is already
81-
// complete, preventing use-after-free races.
82-
Server *s = server.exchange(nullptr, std::memory_order_acq_rel);
83-
if (s != nullptr) {
84-
s->close();
85-
}
8670
Action::cancel(cont);
71+
server->close();
72+
}
73+
74+
Continuation *
75+
operator=(Continuation *acont)
76+
{
77+
return Action::operator=(acont);
8778
}
8879

8980
~NetAcceptAction() override

src/iocore/net/QUICNetProcessor.cc

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,9 @@ QUICNetProcessor::main_accept(Continuation *cont, SOCKET fd, AcceptOptions const
251251
na->server.sock = UnixSocket{fd};
252252
ats_ip_copy(&na->server.accept_addr, &accept_ip);
253253

254-
na->action_ = new NetAcceptAction(cont, &na->server);
254+
na->action_ = new NetAcceptAction();
255+
*na->action_ = cont;
256+
na->action_->server = &na->server;
255257
na->init_accept();
256258

257259
return na->action_.get();

src/iocore/net/UnixNetAccept.cc

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -479,7 +479,6 @@ NetAccept::acceptEvent(int event, void *ep)
479479
MUTEX_TRY_LOCK(lock, m, e->ethread);
480480
if (lock.is_locked()) {
481481
if (action_->cancelled) {
482-
// Server was already closed by whoever called cancel().
483482
e->cancel();
484483
Metrics::Gauge::decrement(net_rsb.accepts_currently_open);
485484
delete this;
@@ -488,7 +487,6 @@ NetAccept::acceptEvent(int event, void *ep)
488487

489488
int res;
490489
if ((res = net_accept(this, e, false)) < 0) {
491-
action_->cancel();
492490
Metrics::Gauge::decrement(net_rsb.accepts_currently_open);
493491
/* INKqa11179 */
494492
Warning("Accept on port %d failed with error no %d", ats_ip_port_host_order(&server.addr), res);
@@ -639,7 +637,7 @@ NetAccept::acceptFastEvent(int event, void *ep)
639637
return EVENT_CONT;
640638

641639
Lerror:
642-
action_->cancel();
640+
server.close();
643641
e->cancel();
644642
Metrics::Gauge::decrement(net_rsb.accepts_currently_open);
645643
delete this;
@@ -658,7 +656,6 @@ NetAccept::acceptLoopEvent(int event, Event *e)
658656
}
659657

660658
// Don't think this ever happens ...
661-
action_->cancel();
662659
Metrics::Gauge::decrement(net_rsb.accepts_currently_open);
663660
delete this;
664661
return EVENT_DONE;

src/iocore/net/UnixNetProcessor.cc

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,9 @@ UnixNetProcessor::accept_internal(Continuation *cont, int fd, AcceptOptions cons
133133
na->proxyPort = sa ? sa->proxyPort : nullptr;
134134
na->snpa = dynamic_cast<SSLNextProtocolAccept *>(cont);
135135

136-
na->action_ = new NetAcceptAction(cont, &na->server);
136+
na->action_ = new NetAcceptAction();
137+
*na->action_ = cont;
138+
na->action_->server = &na->server;
137139

138140
if (opt.frequent_accept) { // true
139141
if (accept_threads > 0 && listen_per_thread == 0) {

0 commit comments

Comments
 (0)