acassen / keepalived

Keepalived
https://www.keepalived.org
GNU General Public License v2.0
4.01k stars 736 forks source link

Possible bad copy paste in track_process.c #2425

Closed Bbulatov closed 5 months ago

Bbulatov commented 5 months ago

Hello! During the static analysis, was found one possible mistake (bad copy past) in code. Variable tpr->terminate_delay is using and checking in file keepalived/core/track_process.c:905-909. Variable tpr->fork_delay is checking in keepalived/core/track_process.c:895, but in 896 used variable tpr->terminate_delay. Perhaps you should use tpr->fork_delay instead of tpr->terminate_delay in line 896.

изображение

Please clarify whether this needs a fix?

pqarmitage commented 5 months ago

@Bbulatov Many thanks for reporting this. I think, however, that the problem is worse than you identify, meaning that currently whatever value of the timer is used is irrelevant.

The code at lines 895-897, currently:

                                if (tpr->fork_delay)
                                        tpr->fork_timer_thread = thread_add_timer(master, process_gained_quorum_timer_thread, tpr, tpr->terminate_delay);
                                process_update_track_process_status(tpr, true);

I think should be:

                                if (tpr->fork_delay)
                                        tpr->fork_timer_thread = thread_add_timer(master, process_gained_quorum_timer_thread, tpr, tpr->fork_delay);
                                else
                                        process_update_track_process_status(tpr, true);

In other words, not only is the delay timer the wrong one, as you identified, but _process_update_track_processstatus() should not be called if there is a fork_delay configured.

I need to think this through carefully and do some testing before producing a patch.

pqarmitage commented 5 months ago

Commit 3df2947 resolves this issue, and a couple of others I identified while investigating it.