Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#19454 closed defect (fixed)

tor crashed when "HeartbeatPeriod 0"

Reported by: kukabu Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.4-rc
Severity: Blocker Keywords: review-group-4
Cc: Actual Points:
Parent ID: Points:
Reviewer: asn Sponsor:

Description

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)

Child Tickets

Change History (18)

comment:1 Changed 3 years ago by asn

Keywords: 029-proposed added
Milestone: Tor: unspecifiedTor: 0.2.???
Points: 0.2
Severity: CriticalNormal

Triaging this.

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

comment:2 Changed 3 years ago by cypherpunks

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

comment:3 Changed 3 years ago by Sebastian

Milestone: Tor: 0.2.???Tor: 0.2.8.x-final
Points: 0.2
Severity: NormalBlocker
Version: Tor: unspecifiedTor: 0.2.8.4-rc

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.

comment:4 Changed 3 years ago by 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) {

comment:5 Changed 3 years ago by kukabu

comment:6 in reply to:  5 Changed 3 years ago by 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).

comment:7 Changed 3 years ago by nickm

Status: newneeds_review

comment:8 Changed 3 years ago by asn

Reviewer: asn

comment:9 Changed 3 years ago by nickm

Keywords: 029-proposed removed

comment:10 Changed 3 years ago by nickm

Keywords: review-group-4 added

comment:11 Changed 3 years ago by 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.

comment:12 Changed 3 years ago by 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?

comment:13 Changed 3 years ago by dgoulet

lgtm!

comment:14 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

cherry-picked to maint-0.2.8 and merged forward.

comment:15 Changed 3 years ago by 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.

comment:16 in reply to:  15 Changed 3 years ago by 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.

comment:17 in reply to:  15 ; Changed 3 years ago by 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!

comment:18 in reply to:  17 Changed 3 years ago by 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.

Note: See TracTickets for help on using tickets.