Opened 3 years ago

Closed 3 years ago

#18686 closed defect (fixed)

tor port forwarding claims to kill long-dead forwarder

Reported by: chadmiller Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7.6
Severity: Major Keywords: 027-backport
Cc: Actual Points:
Parent ID: Points: small
Reviewer: Sponsor:

Description

Mar 30 09:36:07 h Tor[1243]: tor_check_port_forwarding(): Started port forwarding helper (.../bin/tor-fw-helper) with pid '5936'
Mar 30 09:36:09 h Tor[1243]: notify_waitpid_callback_by_pid(): Child process 5936 has exited; running callback.
Mar 30 09:41:13 h Tor[1243]: Failed to terminate process with PID '5936' ('Success').

Mar 30 09:41:13 h Tor[1243]: tor_check_port_forwarding(): Started port forwarding helper (...bin/tor-fw-helper) with pid '5985'
Mar 30 09:41:15 h Tor[1243]: notify_waitpid_callback_by_pid(): Child process 5985 has exited; running callback.
Mar 30 09:46:19 h Tor[1243]: Failed to terminate process with PID '5985' ('Success').

Mar 30 09:46:19 h Tor[1243]: tor_check_port_forwarding(): Started port forwarding helper (.../bin/tor-fw-helper) with pid '6052'
Mar 30 09:46:21 h Tor[1243]: notify_waitpid_callback_by_pid(): Child process 6052 has exited; running callback.
Mar 30 09:51:25 h Tor[1243]: Failed to terminate process with PID '6052' ('Success').

Mar 30 09:51:25 h Tor[1243]: tor_check_port_forwarding(): Started port forwarding helper (.../bin/tor-fw-helper) with pid '8784'
Mar 30 09:51:27 h Tor[1243]: notify_waitpid_callback_by_pid(): Child process 8784 has exited; running callback.
Mar 30 09:56:31 h Tor[1243]: Failed to terminate process with PID '8784' ('Success').

Mar 30 09:56:31 h Tor[1243]: tor_check_port_forwarding(): Started port forwarding helper (.../bin/tor-fw-helper) with pid '8848'
Mar 30 09:56:33 h Tor[1243]: notify_waitpid_callback_by_pid(): Child process 8848 has exited; running callback.
Mar 30 10:01:37 h Tor[1243]: Failed to terminate process with PID '8848' ('Bad file descriptor').

tor_check_port_forwarding in util.c keeps a static process_handle_t . Before spawning the forwarder helper, it says it tries to kill previously-running instances.

The cause is that the killing function doesn't distinguish between between not needing to kill versus attempted and failed.

The resulting error message has a ugly, wrong error reason too.

Child Tickets

Attachments (1)

tor-bug-18686 (2.8 KB) - added by chadmiller 3 years ago.
proposed fix

Download all attachments as: .zip

Change History (12)

Changed 3 years ago by chadmiller

Attachment: tor-bug-18686 added

proposed fix

comment:1 Changed 3 years ago by chadmiller

This tries to fix the problem. Move the logging closer to the failure, and distinguish between not wanting/needing to kill versus failing to kill.

comment:2 Changed 3 years ago by nickm

Milestone: Tor: 0.2.8.x-final
Status: newneeds_review

comment:3 Changed 3 years ago by nickm

Would this patch be sufficient?

diff --git a/src/common/util.c b/src/common/util.c
index 2351faf5036e44..b932d7310f97de 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -3949,7 +3949,7 @@ tor_terminate_process(process_handle_t *process_handle)
   }
 #endif
 
-  return -1;
+  return 0; /* We decided not to kill the process, so report success. */
 }
 
 /** Return the Process ID of <b>process_handle</b>. */

comment:4 Changed 3 years ago by nickm

Owner: set to nickm
Status: needs_reviewaccepted

set owner.

comment:5 Changed 3 years ago by nickm

Status: acceptedneeds_review

comment:6 in reply to:  3 Changed 3 years ago by dgoulet

Replying to nickm:

Would this patch be sufficient?

diff --git a/src/common/util.c b/src/common/util.c
index 2351faf5036e44..b932d7310f97de 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -3949,7 +3949,7 @@ tor_terminate_process(process_handle_t *process_handle)
   }
 #endif
 
-  return -1;
+  return 0; /* We decided not to kill the process, so report success. */
 }
 
 /** Return the Process ID of <b>process_handle</b>. */

I'm not entirely sure here. Returning 0 indicate that a termination signal has been sent to the process. It's what the caller should expect because kill(2) returns 0 on success with "at least one signal was sent" guarantee.

It would indeed fix the issue I believe but it just seems wrong and might be error prone in the future. Lucky for us, there is only one callsite for tor_terminate_process so what about it returns 1 indicating that nothing was done. 0 would be the success and -1 a syscall error?

comment:7 Changed 3 years ago by nickm

Keywords: TorCoreTeam201605 added

These 0.2.8 items really should get handled in May. Or earlier.

comment:8 Changed 3 years ago by nickm

Calling all non-needs_information tickets for May.

comment:9 Changed 3 years ago by nickm

Points: small

comment:10 Changed 3 years ago by nickm

Keywords: 027-backport added; TorCoreTeam201605 removed
Milestone: Tor: 0.2.8.x-finalTor: 0.2.7.x-final

I think a comment change would cover that then. We just document that tor_terminate_process means "kill the process if it has not already exited." One way to succeed at that is to do nothing, because it already exited.

I've taken that approach (better documenting what we do) in branch bug18686_025, which I've merged to 0.2.8.

comment:11 Changed 3 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final
Resolution: fixed
Status: needs_reviewclosed

Marking these items as "no backport planned in 0.2.7". Since there is a newer stable, and these aren't security holes or major stability problems, they don't get a backport.

Note: See TracTickets for help on using tickets.