Opened 10 months ago

Closed 8 months ago

#24588 closed defect (implemented)

Make signal handlers optional, for starting Tor in-process

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-mobile, s8-api, review-group-30
Cc: darkk, brade, mcs, sbs, mtigas Actual Points:
Parent ID: #23684 Points:
Reviewer: Sponsor: Sponsor8

Description

When Tor starts, it installs handlers for a bunch of signals. But if you're running Tor in its own thread, there's a good chance you don't actually want that behavior.

Child Tickets

Change History (9)

comment:1 Changed 9 months ago by nickm

I've documented this feature in a torspec branch called disable_signal_handlers.

The implementation is in a tor branch called disable_signal_handlers.

comment:2 Changed 9 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:3 Changed 9 months ago by nickm

Status: acceptedneeds_review

comment:4 Changed 8 months ago by nickm

Keywords: review-group-30 added
Milestone: Tor: 0.3.4.x-finalTor: 0.3.2.x-final

comment:5 Changed 8 months ago by mikeperry

Status: needs_reviewneeds_revision

Why is this listed for controllers in control-spec.txt if you can't SETCONF/change the value while Tor is running (as per the check added to options_transition_allowed)?

If there is another way for controllers to use this other than SETCONF, probably the control-spec.txt should say to do that (and not SETCONF). If not, then this should go into the manpage (and probably should go into the manpae anyway).

I was also briefly confused what the event handler was doing in the else where signal handlers are set to disabled. Maybe document that it is for the SIGNAL control port command/activate_signals()?

I think the rest of this looks OK though. Tests pass w/ hardening, spaces/changes OK, etc.

comment:6 Changed 8 months ago by nickm

Why is this listed for controllers in control-spec.txt if you can't SETCONF/change the value while Tor is running (as per the check added to options_transition_allowed)?

That's because it's meant for use by controllers only, not in a regular torrc. If you're not embedding Tor, this option isn't useful. We already have similar controller-only unchangeable options in control-spec.txt, like OwningControllerFD.

I was also briefly confused what the event handler was doing in the else where signal handlers are set to disabled. Maybe document that it is for the SIGNAL control port command/activate_signals()?

Okay! I've added a commit to improve the documentation in general.

comment:7 Changed 8 months ago by nickm

Status: needs_revisionneeds_review

comment:8 Changed 8 months ago by mikeperry

Ok, this documentation for the code looks great. I still think it is surprising to list things in controller-spec.txt that can't be set via SETCONF. The first paragraph of that section says "These options can be set and examined by the SETCONF and GETCONF commands.."

How about this fixup so that it is not as much of a surprise:
https://gitweb.torproject.org/user/mikeperry/torspec.git/commit/?h=disable_signal_handlers&id=0d95894f3ae46009d0e3f5dddcbe3f221d8a6f1e

That branch also has a bonus commit for OwningControllerFD:
https://gitweb.torproject.org/user/mikeperry/torspec.git/commit/?h=disable_signal_handlers&id=cde44d7276bef1ec4c538b5f585317c4a1ddeca5

comment:9 Changed 8 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Great! I squashed those in, and added d6ff5e71e305f5baef1df6676e0690be6ba705c9 to fix the first paragraph. I've merged that to torspec master, and merged the code branch to Tor master. Thanks for the review!

Note: See TracTickets for help on using tickets.