Opened 7 months ago

Closed 7 months ago

Last modified 7 months ago

#25252 closed defect (fixed)

Rust implementation of protover code deviates from C in some cases

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 033-must protover rust
Cc: teor Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Teor found some cases where the Rust protover code deviates from the C protover code. We should fix all of these in 0.3.3.

Child Tickets

Change History (4)

comment:1 Changed 7 months ago by nickm

Priority: MediumHigh
Status: assignedneeds_review

Update: most of the issues here are solvable with a little tweaking. I've made changes in torspec in a8455f4d1a6ef7 to define the C behavior as "right" a little more.

The one issue that will be trickiest to handle is that the Rust code stores versions in a hashset of u32, whereas the C code stores an array of (lo,hi) pairs. As such, the Rust code needs to reject 1-4000000000 as a range, or else it will fill up RAM. C accepts such a range everywhere except during voting.

I propose that we handle that last issue separately from the others.

The easier issues are solved in my branch protover_rust_compat_v3, along with a fix for #25250.

comment:2 Changed 7 months ago by teor

Status: needs_reviewmerge_ready

This looks great!

Before we run this code on the public authorities, we should fuzz it to make sure it matches (see #24265).

comment:3 Changed 7 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

merging!

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

Replying to nickm:

Update: most of the issues here are solvable with a little tweaking. I've made changes in torspec in a8455f4d1a6ef7 to define the C behavior as "right" a little more.

The one issue that will be trickiest to handle is that the Rust code stores versions in a hashset of u32, whereas the C code stores an array of (lo,hi) pairs. As such, the Rust code needs to reject 1-4000000000 as a range, or else it will fill up RAM. C accepts such a range everywhere except during voting.

I propose that we handle that last issue separately from the others.

...

That last issue already has a ticket: #24031.

Note: See TracTickets for help on using tickets.