Opened 20 months ago

Last modified 3 months ago

#28081 needs_review defect

rust protover discards all votes if one is not UTF-8

Reported by: cyberpunks Owned by:
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version: Tor: 0.3.3.1-alpha
Severity: Normal Keywords: rust, protover, 035-deferred-20190115, 041-proposed, 033-unreached-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: ahf Sponsor:

Description


Child Tickets

Change History (19)

comment:1 Changed 20 months ago by cyberpunks

Got a fix for this but it conflicts with #27741.

comment:2 Changed 20 months ago by nickm

Milestone: Tor: 0.3.5.x-final

comment:3 Changed 20 months ago by teor

Keywords: 033-backport 034-backport added

comment:4 Changed 20 months ago by teor

Do you want to wait until #27741 is merged?

comment:5 Changed 19 months ago by teor

Status: newneeds_revision

#27741 has been merged.
Which branch do you want us to review?
Does it need to be rebased?

comment:6 Changed 17 months ago by nickm

Keywords: 035-deferred-20190115 041-proposed added
Milestone: Tor: 0.3.5.x-finalTor: unspecified

Marking a number of 0.3.5 tickets as possible, maybe even a good idea, for later. Possibly backportable, some of them. But not currently things to do as part of 0.3.5 stabilization.

comment:7 Changed 15 months ago by teor

Keywords: 033-backport removed

These open, non-merge_ready tickets can not get backported to 0.3.3, because 0.3.3 is now unsupported.

comment:8 Changed 15 months ago by teor

Keywords: 033-backport-unreached added

Hmm, I guess they should still get 033-backport-unreached

comment:9 Changed 12 months ago by nickm

Keywords: 034-backport removed

Removing 034-backport from all open tickets: 034 has reached EOL.

comment:10 Changed 12 months ago by teor

Status: needs_revisionnew

We don't know which branch to review or revise for this ticket.

comment:11 Changed 10 months ago by teor

Keywords: 033-unreached-backport added; 033-backport-unreached removed

Fix 033-unreached-backport spelling.

comment:12 in reply to:  5 Changed 4 months ago by cypherpunks

Replying to teor:

#27741 has been merged.
Which branch do you want us to review?

Branch here:

https://gitgud.io/onionk/tor/compare/master...rust-protover-unicode

comment:13 Changed 4 months ago by teor

Milestone: Tor: unspecifiedTor: 0.4.4.x-final
Status: newneeds_review

comment:14 Changed 4 months ago by dgoulet

Reviewer: ahf

comment:15 Changed 4 months ago by ahf

Status: needs_reviewneeds_revision

Is skipping "bad" entries in the Smartlist, silently, really a good option here?

Also, please submit this as a Github PR such that we can make sure our CI is run.

comment:16 Changed 4 months ago by teor

Tor should log when it rejects votes because of protover issues, like it does when it rejects votes for other reasons.

Ideally, the votes should be rejected when they are uploaded, so that the remote authority also gets a log. But if that's not possible, that's ok.

Last edited 4 months ago by teor (previous) (diff)

comment:17 in reply to:  15 Changed 3 months ago by cypherpunks

Replying to ahf:

Is skipping "bad" entries in the Smartlist, silently, really a good option here?

It's definitely a much better option than the current behavior of silently discarding all entries, isn't it?

This was just supposed to be a simple bugfix, without going into the weeds of more extensive improvements and refactoring this area of the code. (Adding logging really requires refactoring.)

If you prefer to do them all at once instead, added logging and other improvements here:

https://gitgud.io/onionk/tor/compare/master...rust-protover-unicode3

comment:18 in reply to:  16 Changed 3 months ago by cypherpunks

Replying to teor:

Ideally, the votes should be rejected when they are uploaded, so that the remote authority also gets a log.

That rejection is added as part of #27194.

comment:19 Changed 3 months ago by teor

Status: needs_revisionneeds_review
Note: See TracTickets for help on using tickets.