Opened 5 months ago

Last modified 42 hours ago

#27197 merge_ready defect

rust protover accepts excess commas in version strings

Reported by: cyberpunks Owned by:
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version: Tor: 0.3.3.3-alpha
Severity: Normal Keywords: rust, security-low, 033-backport, 034-backport
Cc: Actual Points:
Parent ID: #27194 Points:
Reviewer: teor Sponsor:

Description


Child Tickets

Change History (14)

comment:1 Changed 5 months ago by cyberpunks

See the rust-protocommas1 branch at https://gitgud.io/onionk/tor.git based on maint-0.3.3.

This was introduced in rust protover: match the C implementation on empty-str cases.

comment:2 Changed 5 months ago by teor

Status: newneeds_review

comment:3 Changed 4 months ago by asn

Reviewer: teor

comment:5 Changed 3 months ago by ahf

Status: needs_reviewmerge_ready

LGTM.

Whoever merges this: Rust is not not my biggest strength. Please have a look as well.

comment:6 Changed 3 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

merged to 0.3.5 and forward.

comment:7 in reply to:  6 Changed 3 months ago by cyberpunks

Replying to nickm:

merged to 0.3.5 and forward.

But this bug exists in maint-0.3.3.

comment:8 Changed 3 months ago by nickm

Milestone: Tor: 0.3.5.x-finalTor: 0.3.4.x-final
Resolution: fixed
Status: closedreopened

Okay, we can consider a backport.

comment:9 Changed 3 months ago by nickm

Status: reopenedmerge_ready

comment:10 Changed 3 months ago by teor

The 0.3.3 pull request for this branch is:
https://github.com/torproject/tor/pull/428

comment:11 in reply to:  10 Changed 3 months ago by cyberpunks

Replying to teor:

The 0.3.3 pull request for this branch is:
https://github.com/torproject/tor/pull/428

Was github having an issue with CI? CI ran on PR 431 but not on PRs 427 to 430.

comment:12 Changed 3 months ago by teor

GitHub had major downtime on some systems, including CI jobs. When they started processing jobs again, they dropped some of them:

As we re-enabled processing of this data, we processed ~200,000 webhook payloads that had outlived an internal TTL and were dropped.

https://blog.github.com/2018-10-30-oct21-post-incident-analysis/

comment:13 Changed 2 weeks ago by nickm

I'm still going to suggest "no backport" here -- people who want to try Rust builds should IMO stick with recent versions.

comment:14 in reply to:  13 Changed 42 hours ago by cypherpunks3

Replying to nickm:

people who want to try Rust builds should IMO stick with recent versions.

The parent ticket #27194 fixes the same bug in C code, as well as adding unit tests, and those new tests will fail CI if the rust code isn't also fixed.

In order to merge the C fix to 0.3.3 you either need the Rust fix in 0.3.3 too or to give up on unit testing.

Note: See TracTickets for help on using tickets.