handling double spaces in protover

protover.c accepts trailing spaces and extra spaces between subprotocol entries like "Link=1-4 LinkAuth=1 ", but rejects leading spaces like " Link=1-4". It has since its introduction.

The Rust implementation rejects all extra spaces in any position. It's at least consistent.

Trac:
Username: cyberpunks

changed milestone to %Tor: unspecified

added 032-unreached-backport 033-unreached-backport 035-deferred-20190115 041-proposed component::core tor/tor milestone::Tor: unspecified priority::medium reporter::cyberpunks reviewer::nickm rust severity::normal status::needs-revision type::defect version::tor 0.2.9.4-alpha labels

The protospaces2 branch at ​​https://gitgud.io/onionk/tor.git changes the C implementation to reject them. Nothing actually produces extra spaces like this, right?

Trac:
Username: cyberpunks

Replying to cyberpunks:

The protospaces2 branch at ​​https://gitgud.io/onionk/tor.git changes the C implementation to reject them. Nothing actually produces extra spaces like this, right?

Extra spaces are not allowed by the protover spec. Each separator is exactly one space: https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n780

But we should run make test-network-all using chutney to make sure that standard tor clients aren't inserting extra spaces.

Trac:
Status: new to needs_review

Trac:
Keywords: N/A deleted, 034-backport, 029-backport, 033-backport, 032-backport added
Milestone: N/A to Tor: 0.3.5.x-final

We could also update the protover spec to allow any number of spaces and tabs (WS), rather than a single space (SP), and update the C and Rust implementations to match.

That would be more consistent with Tor's directory document meta-format: https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n193

I'd like to get nickm's opinion before we move forward.

Trac:
Status: needs_review to needs_information

Also, I'm not sure how you're finding all these protover bugs, but you might be interested in protover fuzzing in #24265 (moved). Someone already tried in #26265 (moved), but they didn't do it using Tor's existing fuzzing infrastructure, and we needed to be able to fuzz the rust and C functions in the same process.

If I cc nickm, I'm more likely to get his opinion.

Trac:
Cc: N/A to nickm

Visual inspection of the code made them pretty apparent.

Also, it's been 3 weeks now. Is this gonna get reviewed before Friday?

Trac:
Username: cyberpunks

Replying to cyberpunks:

Visual inspection of the code made them pretty apparent.

Also, it's been 3 weeks now. Is this gonna get reviewed before Friday?

We aim to review code within a week, but we've been busy with the 0.3.4 release: https://lists.torproject.org/pipermail/tor-announce/2018-September/000164.html And the 0.3.5 code freeze: https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/CoreTorReleases

If it helps, you're not the only person waiting for code reviews: https://trac.torproject.org/projects/tor/query?status=needs_review&component=Core+Tor%2FTor&col=id&col=summary&col=status&col=type&col=priority&col=milestone&col=component&order=priority

I'm still waiting on nickm's opinion on spec vs code changes:

Replying to teor:

We could also update the protover spec to allow any number of spaces and tabs (WS), rather than a single space (SP), and update the C and Rust implementations to match.

That would be more consistent with Tor's directory document meta-format: https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n193

I'd like to get nickm's opinion before we move forward.

I'll try setting him as reviewer on this ticket instead.

Trac:
Reviewer: N/A to nickm
Status: needs_information to needs_review
Cc: nickm to N/A

Sorry for the delay -- I do think we should use the metaformat from the rest of the spec, and change the spec to WS here.

Trac:
Status: needs_review to needs_revision

0.3.2 is end of life, so 032-backport is now 032-unreached-backport.

Trac:
Keywords: 032-backport deleted, 032-unreached-backport added

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.

Trac:
Keywords: N/A deleted, 041-proposed, 035-deferred-20190115 added
Milestone: Tor: 0.3.5.x-final to Tor: unspecified

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

Trac:
Keywords: 033-backport deleted, N/A added

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

Trac:
Keywords: N/A deleted, 033-backport-unreached added

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

Trac:
Keywords: 034-backport deleted, N/A added

Fix 033-unreached-backport spelling.

Trac:
Keywords: 033-backport-unreached deleted, 033-unreached-backport added

0.2.9 is no longer maintained as of 1 January 2020. Therefore, we won't be backporting anything to it.

(None of these tickets are merge_ready, so we can just delete the backport tag.)

Trac:
Keywords: 029-backport deleted, N/A added

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