tor port forwarding claims to kill long-dead forwarder

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.

Trac:
Username: chadmiller

changed milestone to %Tor: 0.2.8.x-final

added 027-backport component::core tor/tor milestone::Tor: 0.2.8.x-final owner::nickm points::small priority::medium reporter::chadmiller resolution::fixed severity::major status::closed type::defect version::tor 0.2.7.6 labels

Trac:
Username: chadmiller

tor-bug-18686

proposed fix

Trac:
Username: 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.

Trac:
Username: chadmiller

Trac:
Milestone: N/A to Tor: 0.2.8.x-final
Status: new to needs_review

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>. */

set owner.

Trac:
Status: needs_review to accepted
Owner: N/A to nickm

Trac:
Status: accepted to needs_review

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 process_handle. */ }}}

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?

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

Trac:
Keywords: N/A deleted, TorCoreTeam201605 added

Calling all non-needs_information tickets for May.

Trac:
Points: N/A to small

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.

Trac:
Milestone: Tor: 0.2.8.x-final to Tor: 0.2.7.x-final
Keywords: TorCoreTeam201605 deleted, 027-backport added

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.

Trac:
Milestone: Tor: 0.2.7.x-final to Tor: 0.2.8.x-final
Status: needs_review to closed
Resolution: N/A to fixed

closed

moved to tpo/core/tor#18686 (closed)