Skip to content

Commit

Permalink
rec: Fix a potential null deref in MTasker::schedule()
Browse files Browse the repository at this point in the history
The bug is located in a part of the code that we never actually
use since we always pass the current time to the function, so
I decided to reduce the complexity by making this parameter mandatory.

Reported by Coverity as CID 1533199.
  • Loading branch information
rgacogne committed Jan 9, 2024
1 parent 369b105 commit c87f074
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 16 deletions.
15 changes: 3 additions & 12 deletions pdns/recursordist/mtasker.hh
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public:
void yield();
int sendEvent(const EventKey& key, const EventVal* val = nullptr);
void makeThread(tfunc_t* start, void* val);
bool schedule(const struct timeval* now = nullptr);
bool schedule(const struct timeval& now);

const waiters_t& getWaiters() const
{
Expand Down Expand Up @@ -395,7 +395,7 @@ void MTasker<Key, Val, Cmp>::makeThread(tfunc_t* start, void* val)
*/
template <class Key, class Val, class Cmp>
bool MTasker<Key, Val, Cmp>::schedule(const struct timeval* now)
bool MTasker<Key, Val, Cmp>::schedule(const struct timeval& now)
{
if (!d_runQueue.empty()) {
d_tid = d_runQueue.front();
Expand Down Expand Up @@ -433,21 +433,12 @@ bool MTasker<Key, Val, Cmp>::schedule(const struct timeval* now)
return true;
}
if (!d_waiters.empty()) {
struct timeval rnow
{
};
if (now != nullptr) {
gettimeofday(&rnow, nullptr);
}
else {
rnow = *now;
}
typedef typename waiters_t::template index<KeyTag>::type waiters_by_ttd_index_t;
// waiters_by_ttd_index_t& ttdindex=d_waiters.template get<KeyTag>();
waiters_by_ttd_index_t& ttdindex = boost::multi_index::get<KeyTag>(d_waiters);

for (typename waiters_by_ttd_index_t::iterator i = ttdindex.begin(); i != ttdindex.end();) {
if (i->ttd.tv_sec && i->ttd < rnow) {
if (i->ttd.tv_sec && i->ttd < now) {
d_waitstatus = TimeOut;
d_eventkey = i->key; // pass waitEvent the exact key it was woken for
auto ucontext = i->context;
Expand Down
2 changes: 1 addition & 1 deletion pdns/recursordist/rec-main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2630,7 +2630,7 @@ static void recLoop()
auto& threadInfo = RecThreadInfo::self();

while (!RecursorControlChannel::stop) {
while (g_multiTasker->schedule(&g_now)) {
while (g_multiTasker->schedule(g_now)) {
; // MTasker letting the mthreads do their thing
}

Expand Down
6 changes: 3 additions & 3 deletions pdns/recursordist/test-mtasker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ BOOST_AUTO_TEST_CASE(test_Simple)
bool first = true;
int o = 24;
for (;;) {
while (mt.schedule(&now))
while (mt.schedule(now))

Check warning on line 35 in pdns/recursordist/test-mtasker.cc

View workflow job for this annotation

GitHub Actions / Analyze (cpp, rec)

statement should be inside braces (readability-braces-around-statements - Level=Warning)
;
if (first) {
mt.sendEvent(12, &o);
Expand Down Expand Up @@ -70,7 +70,7 @@ BOOST_AUTO_TEST_CASE(test_AlmostStackOverflow)
bool first = true;
int o = 25;
for (;;) {
while (mt.schedule(&now)) {
while (mt.schedule(now)) {
;
}
if (first) {
Expand Down Expand Up @@ -98,7 +98,7 @@ BOOST_AUTO_TEST_CASE(test_MtaskerException)
now.tv_sec = now.tv_usec = 0;

for (;;) {
mt.schedule(&now);
mt.schedule(now);
}
},
std::exception);
Expand Down

0 comments on commit c87f074

Please sign in to comment.