Opened 2 years ago

Last modified 2 years ago

#21449 new defect

Make tor version parsing and version spec consistent

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-spec version-parse tor-client tor-relay
Cc: Actual Points:
Parent ID: Points: 2
Reviewer: Sponsor:

Description

In tor_version_compare, the git logic is a bit weird, because git tags are hashes: the ordering we apply isn't their true order. So the function comment should probably become:

/** Compare two tor versions; Return <0 if a < b; 0 if a ==b, >0 if a >
* b. Git tags are sorted by length, then value. */

But this doesn't match version-spec.txt:

The EXTRA_INFO is also purely informational, often containing information
about the SCM commit this version came from. It is surrounded by parentheses
and can't contain whitespace. Unlike the STATUS_TAG this never impacts the way
that versions should be compared.

We should try to ignore the git tag, or at least be very careful how we compare it. Due to bugs like #20492, the following versions are equivalent:

Tor 0.2.9.9 (git-56788a2489127072)
Tor 0.2.9.9

(But these are not equivalent to any other git tag on Tor 0.2.9.9, which should never happen anyway.)

This is important when we try to exclude versions, like in #20509, because we need to exclude the version with and without the git tag.

This fix might require a new consensus method.

Child Tickets

TicketStatusOwnerSummaryComponent
#21454assignedtor_version_compare and version spec comparison order are inconsistentCore Tor/Tor

Change History (3)

comment:1 Changed 2 years ago by teor

The svn logic in tor_version_as_new_as doesn't work for git tags:

  /* Here's why we don't need to do any special handling for svn revisions:
   * - If neither has an svn revision, we're fine.
   * - If the router doesn't have an svn revision, we can't assume that it
   *   is "at least" any svn revision, so we need to return 0.
   * - If the target version doesn't have an svn revision, any svn revision
   *   (or none at all) is good enough, so return 1.
   * - If both target and router have an svn revision, we compare them.
   */

comment:2 Changed 2 years ago by teor

Here's how to do an equality check right:
(Every check must be a range check, not an equality check.)
https://trac.torproject.org/projects/tor/ticket/20509#comment:39
https://trac.torproject.org/projects/tor/ticket/20509#comment:40

I think all the current tor code is ok, but it's easy to mess up these comparisons.

We should add a comment to tor_version_compare() recommending using !tor_version_as_new_as(next version) or tor_version_same_series(version), to avoid the git tag issue.

Last edited 2 years ago by teor (previous) (diff)

comment:3 Changed 2 years ago by nickm

Keywords: version-parse tor-client tor-relay added
Note: See TracTickets for help on using tickets.