bug in registering event broker callbacks
Posted: Thu Nov 01, 2018 4:48 pm
I'm looking at nagios-4.4.2/base/nebmods.c, lines 421-422 and the
surrounding code.
The neb_register_callback() routine looks suspect to me in its processing
of priority information as it adds event brokers to the callback lists.
Specifically, if there is already an existing event broker in the
callback list for a particular callback type, and the first iteration
of the loop checking for relative priorities breaks out because the new
callback has priority less than the priority of the first element of
the list, then last_callback remains NULL. In that situation, the new
callback replaces the first element of the callback list, and the "next"
element of the new callback is left as NULL. This effectively orphans the
entire previous callback list of that callback type so those previously
registered callbacks will never be called. What is missing is a
statement immediately before the
statement, so the existing callback list is referred to by the new
callback and the previously existing callback list is not orphaned.
That is to say:
must become:
Going further, no special testing of temp_callback is needed in the
else clause, as this construction handles both the case where the
callback list was previously empty and the case where the callback
list was previously non-empty. In fact, this whole block of code can
then be refactored to remove more special-case handling, take out other
redundancy, and collapse cases. Also, because the interpretation of the
"priority" function is perhaps confusing, here we have also taken the
liberty of reversing the comparison to make it read more intuitively
(compare the new object to the existing object, not the other way round),
and of clarifying in the commment how higher or lower values of priority
affect the later processing. Then for complete clarity as to how and
when last_callback is set, it makes sense to pull those assignments
directly into the for loop clauses. In Nagios 4.4.2 base/nebmods.c,
lines 410-431 get reduced to this much more compact form:
surrounding code.
The neb_register_callback() routine looks suspect to me in its processing
of priority information as it adds event brokers to the callback lists.
Specifically, if there is already an existing event broker in the
callback list for a particular callback type, and the first iteration
of the loop checking for relative priorities breaks out because the new
callback has priority less than the priority of the first element of
the list, then last_callback remains NULL. In that situation, the new
callback replaces the first element of the callback list, and the "next"
element of the new callback is left as NULL. This effectively orphans the
entire previous callback list of that callback type so those previously
registered callbacks will never be called. What is missing is a
Code: Select all
new_callback->next = temp_callback;
Code: Select all
neb_callback_list[callback_type] = new_callback;
callback and the previously existing callback list is not orphaned.
That is to say:
Code: Select all
if(last_callback == NULL)
neb_callback_list[callback_type] = new_callback;
Code: Select all
if(last_callback == NULL) {
new_callback->next = temp_callback;
neb_callback_list[callback_type] = new_callback;
}
else clause, as this construction handles both the case where the
callback list was previously empty and the case where the callback
list was previously non-empty. In fact, this whole block of code can
then be refactored to remove more special-case handling, take out other
redundancy, and collapse cases. Also, because the interpretation of the
"priority" function is perhaps confusing, here we have also taken the
liberty of reversing the comparison to make it read more intuitively
(compare the new object to the existing object, not the other way round),
and of clarifying in the commment how higher or lower values of priority
affect the later processing. Then for complete clarity as to how and
when last_callback is set, it makes sense to pull those assignments
directly into the for loop clauses. In Nagios 4.4.2 base/nebmods.c,
lines 410-431 get reduced to this much more compact form:
Code: Select all
/*
* Add new function to callback list, sorted by priority. Lower "priority" values
* are served first; otherwise, first come, first served for the same priority.
*/
for(last_callback = NULL, temp_callback = neb_callback_list[callback_type];
temp_callback != NULL;
last_callback = temp_callback, temp_callback = temp_callback->next) {
if(new_callback->priority < temp_callback->priority)
break;
}
new_callback->next = temp_callback;
(last_callback != NULL ? last_callback->next : neb_callback_list[callback_type]) = new_callback;