Opened 3 years ago

Last modified 8 months ago

#25605 new defect

Add tests for Rust protover::compute_for_old_tor() and C functions it calls

Reported by: isis Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor:
Severity: Normal Keywords: fast-fix, 033-triage-20180326, 033-included-20180326, 033-removed-20180409, 035-triaged-in-20180711 041-proposed 035-deferred-20190115
Cc: chelseakomlo Actual Points:
Parent ID: #25386 Points: 1
Reviewer: nickm Sponsor:


In, after changing/adding some tests, I noticed that protover::compute_for_old_tor() always returns an empty string, meaning that the version we parsed is new enough that the router should be reporting its own protocol versions. It's because the Rust code is calling the C version of tor_version_as_new_as(), which expects a platform string, not a version, so it gets to tor_version_parse_platform(), decides this is a "non-standard tor" and returns true early. So, in the top of the Rust function protover::compute_for_old_tor(), when we call tor_version_as_new_as(version, FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS) it says true, and we never reach the rest of the function.

The solution is to prepend "Tor " to each of the query strings in protover::compute_for_old_tor().

Child Tickets

Change History (13)

comment:1 Changed 3 years ago by isis

Priority: HighMedium

Ohhhh, actually this is just a problem in the test code, this function is behaving as we expect it to in C. So the fix is to have the tests call it like this protover::compute_for_old_tor("Tor") instead. I wrote some tests for the C in the process.

comment:2 Changed 3 years ago by isis

Status: assignedneeds_review
Summary: Rust protover::compute_for_old_tor() always says the version is newer than tests for Rust protover::compute_for_old_tor() and C functions it calls

Okay the tests are in my bug25605 branch. I'll need to rebase that branch once #25386 is merged.

comment:3 Changed 3 years ago by nickm

Keywords: 033-triage-20180326 added

Second batch of triage for 0.3.3: tickets that we didn't cover the first time.

comment:4 Changed 3 years ago by nickm

Keywords: 033-included-20180326 added

Marking 033-must tickets as included. Round 2.

comment:5 Changed 3 years ago by nickm

Reviewer: nickm

taking reviewer in this one, since I'm already assigned on #24031

comment:6 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

On the routerparse tor_version_parse tests:

  • let's make sure that these tests check the contents of the 'out' structure.
  • You don't need to say "if (foo) tor_free(foo)" -- all of our free functions accept NULL.

Otherwise this looks fine to me. I'm marking it needs_revision because of the two issues above, and because it will need to get rebased onto a later version of #25386.

comment:7 Changed 3 years ago by isis

Keywords: 033-removed-20180409 added; 033-must removed
Milestone: Tor: 0.3.3.x-finalTor: 0.3.5.x-final
Parent ID: #25386

Removing the 033-must tag, since it requires the changes from #25386 (the latter has also just been triaged out of 034 as well). I'll revise this once we've got a solution for #25386, since otherwise this code will fail to link.

Nominating this ticket for inclusion in whatever #25386 is included in, so for now I'm marking as 0.3.5.

comment:8 Changed 2 years ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:9 Changed 2 years ago by dgoulet

Status: needs_revisionnew

comment:10 Changed 2 years ago by dgoulet

Owner: isis deleted
Status: newassigned

comment:11 Changed 22 months ago by gaba

Sponsor: Sponsor8-can

comment:12 Changed 22 months ago by nickm

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

comment:13 Changed 8 months ago by teor

Status: assignednew

Change tickets that are assigned to nobody to "new".

Note: See TracTickets for help on using tickets.