Skip to content

Commit

Permalink
EpollBackend fixes
Browse files Browse the repository at this point in the history
Summary:
EpollBackend fixes for 2 issues:
1) error notifications being sent as READ|WRITE even if WRITE events were not requested - this would lead to errors like:

```
accept_routing_handler_test: fbcode/folly/io/async/AsyncSocket.cpp:3320: virtual void folly::AsyncSocket::handleWrite(): Assertion `writeReqHead_ != nullptr' failed.
```

2) Use after free when receieved events were removed (for example by a timer):

P926145734

The tests mentioned in the Test Plan are working now

Reviewed By: ot

Differential Revision: D52355843

fbshipit-source-id: 0cf32a0f96ac24286881a50013a7329e8f1cefaf
  • Loading branch information
Dan Melnic authored and facebook-github-bot committed Dec 22, 2023
1 parent fcbbb34 commit 4618e42
Showing 1 changed file with 65 additions and 26 deletions.
91 changes: 65 additions & 26 deletions folly/experimental/io/EpollBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <sys/epoll.h>
#include <sys/timerfd.h>

#include <folly/IntrusiveList.h>
#include <folly/String.h>
#include <folly/experimental/io/EpollBackend.h>

Expand All @@ -34,6 +35,26 @@ extern "C" FOLLY_ATTR_WEAK void eb_poll_loop_post_hook(
namespace folly {
namespace {

struct EventInfo {
folly::IntrusiveListHook listHook_;
folly::EventBaseEvent* event_{nullptr};
int what_{0};

void resetEvent() {
// remove it from the list
listHook_.unlink();
if (event_) {
event_ = nullptr;
}
}
};

void eventInfoFreeFunction(void* v) {
delete (EventInfo*)(v);
}

using EventInfoList = folly::IntrusiveList<EventInfo, &EventInfo::listHook_>;

struct SignalRegistry {
struct SigInfo {
struct sigaction sa_ {};
Expand Down Expand Up @@ -172,10 +193,8 @@ EpollBackend::EpollBackend(Options options) : options_(options) {
throw std::runtime_error(folly::errnoStr(errnoCopy));
}

auto callback = [](libevent_fd_t fd, short events, void* arg) {
std::ignore = fd;
std::ignore = events;
reinterpret_cast<EpollBackend*>(arg)->processTimers();
auto callback = [](libevent_fd_t /*fd*/, short /*events*/, void* arg) {
static_cast<EpollBackend*>(arg)->processTimers();
};

timerFdEvent_.eb_event_set(timerFd_, EV_READ | EV_PERSIST, callback, this);
Expand All @@ -187,6 +206,7 @@ EpollBackend::EpollBackend(Options options) : options_(options) {
}

EpollBackend::~EpollBackend() {
eb_event_del(timerFdEvent_);
::close(epollFd_);
::close(timerFd_);
}
Expand Down Expand Up @@ -219,9 +239,11 @@ int EpollBackend::eb_event_base_loop(int flags) {
}

bool shouldProcessTimers = false;
EventInfoList infoList;
for (int i = 0; i < numEvents; ++i) {
struct event* event =
reinterpret_cast<struct event*>(events_[i].data.ptr);
auto* info = static_cast<EventInfo*>(events_[i].data.ptr);
auto* event = info->event_->getEvent();
info->what_ = events_[i].events;
// if not persistent we need to remove it
if (~event->ev_events & EV_PERSIST) {
if (event_ref_flags(event) & EVLIST_INSERTED) {
Expand All @@ -240,7 +262,6 @@ int EpollBackend::eb_event_base_loop(int flags) {

struct epoll_event epev = {};
epev.events = getPollFlags(event->ev_events & (EV_READ | EV_WRITE));
epev.data.ptr = event;

int ret = ::epoll_ctl(epollFd_, EPOLL_CTL_DEL, event->ev_fd, &epev);

Expand All @@ -252,6 +273,7 @@ int EpollBackend::eb_event_base_loop(int flags) {
shouldProcessTimers = true;
} else {
event_ref_flags(event) |= EVLIST_ACTIVE;
infoList.push_back(*info);
}
}

Expand All @@ -260,26 +282,37 @@ int EpollBackend::eb_event_base_loop(int flags) {
processTimers();
}

for (int i = 0; i < numEvents; ++i) {
struct event* event =
reinterpret_cast<struct event*>(events_[i].data.ptr);
while (!infoList.empty()) {
auto* info = &infoList.front();
infoList.pop_front();

if (event->ev_fd == timerFd_) {
continue;
}
struct event* event = info->event_->getEvent();

int what = events_[i].events;
int what = info->what_;
short ev = 0;

bool evRead = (event->ev_events & EV_READ) != 0;
bool evWrite = (event->ev_events & EV_WRITE) != 0;

if (what & EPOLLERR) {
ev = EV_READ | EV_WRITE;
if (evRead) {
ev |= EV_READ;
}
if (evWrite) {
ev |= EV_WRITE;
}
} else if ((what & EPOLLHUP) && !(what & EPOLLRDHUP)) {
ev = EV_READ | EV_WRITE;
if (evRead) {
ev |= EV_READ;
}
if (evWrite) {
ev |= EV_WRITE;
}
} else {
if (what & EPOLLIN) {
if (evRead && (what & EPOLLIN)) {
ev |= EV_READ;
}
if (what & EPOLLOUT) {
if (evWrite && (what & EPOLLOUT)) {
ev |= EV_WRITE;
}
}
Expand Down Expand Up @@ -331,22 +364,24 @@ int EpollBackend::eb_event_add(Event& event, const struct timeval* timeout) {
return 0;
}

if ((ev->ev_events & (EV_READ | EV_WRITE)) &&
!(event_ref_flags(ev) & (EVLIST_INSERTED | EVLIST_ACTIVE))) {
event_ref_flags(ev) |= EVLIST_INSERTED;
numInsertedEvents_++;
}

if (event_ref_flags(ev) & EVLIST_INTERNAL) {
numInternalEvents_++;
} else {
numEvents_++;
}
event_ref_flags(ev) |= EVLIST_INSERTED;
numInsertedEvents_++;

EventInfo* info = static_cast<EventInfo*>(event.getUserData());
if (!info) {
info = new EventInfo();
event.setUserData(info, eventInfoFreeFunction);
}
info->event_ = &event;

struct epoll_event epev = {};
epev.events = getPollFlags(ev->ev_events & (EV_READ | EV_WRITE));
epev.data.ptr = ev;
epev.data.ptr = info;

int ret = ::epoll_ctl(epollFd_, EPOLL_CTL_ADD, ev->ev_fd, &epev);

Expand Down Expand Up @@ -375,6 +410,11 @@ int EpollBackend::eb_event_del(Event& event) {
return 0;
}

auto* info = static_cast<EventInfo*>(event.getUserData());
if (info) {
info->resetEvent();
}

// if the event is on the active list, we just clear the flags
// and reset the event_ ptr
if (event_ref_flags(ev) & EVLIST_ACTIVE) {
Expand All @@ -397,7 +437,6 @@ int EpollBackend::eb_event_del(Event& event) {

struct epoll_event epev = {};
epev.events = getPollFlags(ev->ev_events & (EV_READ | EV_WRITE));
epev.data.ptr = ev;

int ret = ::epoll_ctl(epollFd_, EPOLL_CTL_DEL, ev->ev_fd, &epev);

Expand Down

0 comments on commit 4618e42

Please sign in to comment.