tor crashed when "HeartbeatPeriod 0"

[kukabu]

Hello

tor crashed when "HeartbeatPeriod 0"

periodic_event_dispatch: Bug: Invalid return value for periodic event from heartbeat. (on Tor 0.2.9.0-alpha-dev 1160ac1283a076ac)
tor_assertion_failed_: Bug: src/or/periodic.c:59: periodic_event_dispatch: Assertion r != 0 failed; aborting. (on Tor 0.2.9.0-alpha-dev 1160ac1283a076ac)
Bug: Assertion r != 0 failed in periodic_event_dispatch at src/or/periodic.c:59. Stack trace: (on Tor 0.2.9.0-alpha-dev 1160ac1283a076ac)

[asn]

Triaging this.

This is curious since in the manual page of HeartbeatPeriod we say "Settings this to 0 will disable the heartbeat.".

[cypherpunks]

I think this is caused by ​src/or/main.c:2019. The comment already states that its return value is a bad idea.

[Sebastian]

This looks like a regression in 0.2.8.x that will affect people in the wild, in supported configurations. I tested that it affects 0.2.8.4-rc, the code mentioned by cypherpunks was introduced for 0.2.8.1-alpha. This affects Tor clients and relays alike. I think our policy for such regressions is severity blocker, please correct if I'm wrong.

[cypherpunks]

The following patch seems to fix the issue. The HEARTBEAT_INTERVAL value is a copy of the MIN_HEARTBEAT_PERIOD value in src/or/config.c because its scope prevented me from reusing it. It would be a good idea to write tests for these callback functions to test their intended behavior and catch issues like this in the future.

Also, I'm guessing the XXXX comment is referring to checking the return value of log_heartbeat and changing the return value accordingly. If heartbeats are enabled and the callback is successful, i see nothing wrong with returning the value of HeartbeatPeriod.

diff --git a/src/or/main.c b/src/or/main.c
index 9f3306d..0967248 100644
--- a/src/or/main.c
+++ b/src/or/main.c
@@ -2007,6 +2007,11 @@ check_fw_helper_app_callback(time_t now, const or_options_t *options)
 static int
 heartbeat_callback(time_t now, const or_options_t *options)
 {
+#define HEARTBEAT_INTERVAL (30*60)
+  if (!options->HeartbeatPeriod) {
+    return HEARTBEAT_INTERVAL;
+  }
+
   static int first = 1;
   /* 12. write the heartbeat message */
   if (first) {

[kukabu]

cypherpunks, similar code already exists

​https://gitweb.torproject.org/tor.git/tree/src/or/config.c#n3357

[cypherpunks]

Replying to kukabu:

cypherpunks, similar code already exists

​https://gitweb.torproject.org/tor.git/tree/src/or/config.c#n3357

That is only for checking whether the option (if enabled aka not zero) isn't lower than MIN_HEARTBEAT_PERIOD. My patch changes the callback function to do nothing when heartbeats are disabled (aka HeartbeatPeriod is zero).

[asn]

Hello, please ​see branch bug19454 in my repo:

It's kind of based on cypherpunk's branch above, but it returns PERIODIC_EVENT_NO_UPDATE instead of defining a new magic number and returning that. I think that is more appropriate, but I'm not familiar with the callback system either. I also added a changes file and a function doc.

[asn]

Ah, I also removed this XXX that was saying:

-  /* XXXX This isn't such a good way to handle possible changes in the
-   * callback event */

I'm not sure exactly what was referring to and I thought it was referring to this bug we just fixed, but maybe we should put the XXX back in?

[dgoulet]

lgtm!

[nickm]

cherry-picked to maint-0.2.8 and merged forward.

[cypherpunks]

FWIW the PERIODIC_EVENT_NO_UPDATE causes the callback function to be called every second if i understand the code of periodic_event_dispatch correctly.

[cypherpunks]

Replying to cypherpunks:

FWIW the PERIODIC_EVENT_NO_UPDATE causes the callback function to be called every second if i understand the code of periodic_event_dispatch correctly.

If heartbeats are disabled that is.

[asn]

Replying to cypherpunks:

FWIW the PERIODIC_EVENT_NO_UPDATE causes the callback function to be called every second if i understand the code of periodic_event_dispatch correctly.

We think that's OK. One second is plenty of time for a computer, and that's how our code has been working for ages. Also see other functions that do the same thing (e.g. write_bridge_ns_callback()).

Ideally we would have a better mechanism for scheduling timers, I agree. But we can do this in the future.

Thx for feedback!

[cypherpunks]

Replying to asn:

Replying to cypherpunks:

FWIW the PERIODIC_EVENT_NO_UPDATE causes the callback function to be called every second if i understand the code of periodic_event_dispatch correctly.

We think that's OK. One second is plenty of time for a computer, and that's how our code has been working for ages. Also see other functions that do the same thing (e.g. write_bridge_ns_callback()).

Ideally we would have a better mechanism for scheduling timers, I agree. But we can do this in the future.

Thx for feedback!

I figured it was ok since it was reviewed. Just wanted to state it in case it was overlooked.

I've opened #19476 with a patch that implements my interpretation of the XXXX comment from comment:4.