Opened 6 months ago

Closed 6 months ago

Last modified 6 months ago

#30956 closed defect (fixed)

Publish bridge ServerTransportPlugin lines, even when ExtraInfoStatistics are off

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version: Tor: 0.4.1.1-alpha
Severity: Normal Keywords: tor-bridge, tor-pt, 041-backport, regression, sponsor31-maybe, 041-regression, nickm-merge
Cc: gaba Actual Points: 0.4
Parent ID: #30441 Points: 0.2
Reviewer: nickm Sponsor:

Description

In #29018, we disabled all extrainfo contents when ExtraInfoStatistics is off.

But we need to publish the ServerTransportPlugin lines for bridges, even when their statistics are off.

See https://trac.torproject.org/projects/tor/ticket/30441#comment:13 for details.

Child Tickets

Change History (7)

comment:1 Changed 6 months ago by teor

Actual Points: 0.20.4
Cc: gaba added
Keywords: 041-backport regression sponsor31-maybe added
Status: assignedneeds_review

Gaba, this ticket can go in sponsor 31 for the refactoring, or a pluggable transport sponsor for the bugfix.

See my pull request:

I put the pluggable transport lines straight after the header. This conforms to the spec, because the lines can be in any order. And it makes the code simpler. It also makes the pluggable transport lines less likely to be removed if the extra info descriptor is too big (50 KB, so that's unlikely).

I also did a quick refactor to satisfy practracker on master:

This makes the file bigger, but all the functions are under 100 lines.

comment:2 Changed 6 months ago by nickm

Keywords: 041-regression added

comment:3 Changed 6 months ago by nickm

Keywords: dgoulet-merge added
Reviewer: nickm
Status: needs_reviewmerge_ready

This LGTM; I would like to get it into 0.4.1.3-alpha.

comment:4 Changed 6 months ago by ahf

I think this looks good. Only very minor nitpick I could think of is to change the type of emit_ed_sigs from int to bool, but I don't think that's a blocker.

comment:5 Changed 6 months ago by nickm

Keywords: nickm-merge added; dgoulet-merge removed

okay. now that ahf has reviewed it too, I can merge. Thanks, everybody!

comment:6 Changed 6 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged appropriate branches to 0.4.1 and master.

comment:7 in reply to:  4 Changed 6 months ago by teor

Replying to ahf:

I think this looks good. Only very minor nitpick I could think of is to change the type of emit_ed_sigs from int to bool, but I don't think that's a blocker.

Yeah, I wouldn't do that in the bugfix, but I guess I could do that after the refactor.

Note: See TracTickets for help on using tickets.