Opened 6 months ago

Last modified 6 months ago

#33104 merge_ready defect

Minor issues when handling ACTIVE control signal

Reported by: asn Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version: Tor: 0.4.0.1-alpha
Severity: Normal Keywords: consider-backport-after-0434 tor-dormant 041-backport 042-backport 043-should BugSmashFund
Cc: Actual Points: .1
Parent ID: Points: 0.2
Reviewer: asn Sponsor:

Description (last modified by asn)

The ACTIVE control signal is not handled in control_event_signal() which results in:
control_event_signal(): Bug: Unrecognized signal 132 in control_event_signal messages when it appears.

There is also a mistype in the following comment /* "SIGACTIVE" counts as ersatz user activity. *

Child Tickets

Change History (14)

comment:1 Changed 6 months ago by asn

Description: modified (diff)

comment:2 Changed 6 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:3 Changed 6 months ago by nickm

Actual Points: .1
Keywords: 041-backport? 042-backport? added
Status: acceptedneeds_review

Branch is bug33104_041 with PR at https://github.com/torproject/tor/pull/1696 . Should merge forward cleanly to master.

comment:4 Changed 6 months ago by asn

Reviewer: asn

comment:5 Changed 6 months ago by asn

Looks good! That said, the new code (and the old code) are untested and apparently a source of bugs. I think writing a quick unittest won't be too hard. Any chance we could do that before merging?

comment:6 Changed 6 months ago by asn

Status: needs_reviewneeds_revision

comment:7 Changed 6 months ago by nickm

Keywords: 043-should added

comment:8 Changed 6 months ago by nickm

Status: needs_revisionneeds_review

I've added a test here, though we shouldn't merge this until CI passes.

comment:9 Changed 6 months ago by nickm

Looks like CI is passing except for the rust lint issue.

comment:10 Changed 6 months ago by asn

Status: needs_reviewmerge_ready

Fantastic! This looks good to me. Marking as merge_ready so that Nick can merge whenever the other Rust fix gets merged.

comment:11 Changed 6 months ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: 0.4.2.x-final

Thank you! Merged to master, marking for backport.

comment:12 Changed 6 months ago by teor

Keywords: consider-backport-after-0434 042-backport BugSmashFund added; 041-backport? 042-backport? removed
Version: Tor: 0.4.0.1-alpha

This seems like a minor missing feature / bug. Let's backport it to the latest stable, but no further. Users who need this feature should upgrade to the latest stable.

If 0.4.3.4 is stable (or rc?), then let's not backport to 0.4.2.

comment:13 Changed 6 months ago by nickm

The argument in favor of a backport is this: there are a pair of features (sending "SIGNAL ACTIVE" and listening for signal events with "SETEVENTS SIGNAL") which a control user might want to use together. But without this patch, every time they do so, they will spam an LD_BUG warning into the logs.

That said, I could go either way on a backport.

comment:14 Changed 6 months ago by teor

Keywords: 041-backport added

Oh right. So some application might spam the logs, eat the user's disk, and die?

That's probably worth a backport all the way to 0.4.1, it's only one extra release.

Note: See TracTickets for help on using tickets.