Skip to content

Order prerelease numeric identifiers by value, not lexically - #323

Open
feiiiiii5 wants to merge 1 commit into
Masterminds:masterfrom
feiiiiii5:fix/prerelease-numeric-compare
Open

feiiiiii5 wants to merge 1 commit into
Masterminds:masterfrom
feiiiiii5:fix/prerelease-numeric-compare

Conversation

@feiiiiii5

Copy link
Copy Markdown

Numeric prerelease identifiers are ordered as text once they pass 2^64-1, so Compare returns the opposite of what the spec requires.

The bug

1.0.0-99999999999999999999  >  1.0.0-100000000000000000000   // Compare says greater

Both versions parse without complaint. Numerically 99999999999999999999 is the smaller one; lexically "9" > "1", so it sorts after. Spec item 11 requires numeric identifiers to be compared numerically, and there is no upper bound on them.

Root cause

comparePrePart decided "numeric vs alphanumeric" with strconv.ParseUint:

ParseUint conflates two errors
oi, n1 := strconv.ParseUint(o, 10, 64)
si, n2 := strconv.ParseUint(s, 10, 64)

// The case where both are strings compare the strings
if n1 != nil && n2 != nil {

ParseUint signals two different things through the same error: a syntax error for something that is not a number, and strconv.ErrRange for a number too large for a uint64. Both landed in the string-comparison branch, so an out-of-range numeric identifier was treated as alphanumeric text.

Only identifiers of differing digit counts are affected. -18446744073709551617 vs -18446744073709551616 already compared correctly, because a lexical comparison happens to agree with a numeric one when the widths match — which is why an invariant probe over in-range versions does not surface this.

There is also an inconsistency worth naming: an out-of-range major/minor/patch segment is already rejected at parse time.

Parse already rejects it elsewhere
NewVersion("18446744073709551616.0.0")
  -> error parsing version segment: strconv.ParseUint: parsing "18446744073709551616": value out of range

So the same number is an error in one position and silently mis-sorted in another.

The fix

Identify numeric identifiers by shape rather than by successful parse, then compare them by digit count and digits — no range limit:

The guard
if isNumericIdentifier(s) != isNumericIdentifier(o) {
    // One is a number and one is not, and numeric identifiers always have lower
    // precedence than alphanumeric ones.
    if isNumericIdentifier(s) {
        return -1
    }
    return 1
}

if isNumericIdentifier(s) {
    return compareNumericIdentifiers(s, o)
}
The numeric compare
func compareNumericIdentifiers(s, o string) int {
	s = strings.TrimLeft(s, "0")
	o = strings.TrimLeft(o, "0")
	if len(s) != len(o) {
		if len(s) < len(o) {
			return -1
		}
		return 1
	}
	return strings.Compare(s, o)
}

Leading zeros are stripped so the comparison stays correct for a caller that has not been through validatePrerelease, which rejects them (alpha.01 → ErrSegmentStartsZero). The numeric-below-alphanumeric rule is unchanged, so the alphanumeric path is byte-for-byte what it was.

This is on the comparison path only — no parsing, validation or API change.

Validation

12 cases added to TestCompare, covering both orderings of the differing-width pair, the uint64 boundary, same-width beyond uint64, numeric-versus-alphanumeric at that size, and in-range identifiers that must not move:

Red/green
go test -run TestCompare
  without the fix   FAIL
      Comparison of '1.0.0-99999999999999999999' and '1.0.0-100000000000000000000' failed. Expected '-1', got '1'
      Comparison of '1.0.0-100000000000000000000' and '1.0.0-99999999999999999999' failed. Expected '1', got '-1'
  with the fix    ok

Only those two are red without the change; the other ten pin behaviour that must stay put. The whole suite and the repo's own gates:

Full gate output
make lint    0 issues        (golangci-lint v2, per .golangci.yml)
make test    ok
go test -race ./...                          ok
go test -fuzz FuzzNewVersion -fuzztime 25s   ok
go test -fuzz FuzzStrictNewVersion -fuzztime 25s  ok
go vet ./...    clean
gofmt -l .      clean

I ran all of the above locally, since this repository's workflows are set to action_required for external forks and will need a maintainer to approve them.

The three existing fuzz targets only call the constructors and discard their results, so they would never have caught this — they can find panics, not wrong ordering. A useful follow-up, not part of this change: a fuzz target asserting Compare is a consistent total order, since that is what catches ordering bugs.

Note

I do not know of a real-world version string past 2^64-1 in a prerelease. The argument for fixing it is not that someone has been bitten but that the behaviour is now defined by accident: it depends on uint64 being wide enough, and it contradicts a rule the library otherwise follows exactly. If you would rather reject such identifiers at parse time — which would be consistent with how major/minor/patch segments behave — that is a different fix and I would rather follow your preference than guess.

comparePrePart used strconv.ParseUint and treated any error the same
way, but ParseUint reports two different things: a syntax error for a
non-numeric identifier, and strconv.ErrRange for a number too large for
a uint64. Both fell into the string-comparison branch, so a numeric
prerelease identifier past 2^64-1 was ordered as if it were text:

    1.0.0-99999999999999999999  >  1.0.0-100000000000000000000

because "9" > "1" lexically, while numerically the first is smaller.
Sort order and Compare/LessThan/GreaterThan were all wrong, and both
versions parsed fine.

Compare numeric identifiers by digit count and then by digits, which has
no range limit, and keep the existing numeric-below-alphanumeric rule.
Leading zeros are stripped first so the comparison is still right for a
caller that bypassed validatePrerelease, which rejects them.

Worth noting for consistency: an out-of-range major/minor/patch segment
is already a parse error ("value out of range"), so the same number was
rejected in one position and silently mis-sorted in another.

Pairs of the same width beyond uint64, such as
-18446744073709551617 vs -18446744073709551616, already compared
correctly because a lexical compare agrees with a numeric one there; the
new tests cover both so the distinction stays pinned.
@feiiiiii5

Copy link
Copy Markdown
Author

Hi — picking this back up. Head is f54be222, unchanged since I opened it.

Short version: comparePrePart uses strconv.ParseUint to decide "numeric vs alphanumeric", but ParseUint returns ErrRange (not a syntax error) for a numeric identifier wider than uint64, so those land in the string-comparison branch and sort lexically:

1.0.0-99999999999999999999  >  1.0.0-100000000000000000000   // Compare says greater

Both parse fine, and semver item 11 says numeric identifiers compare numerically with no upper bound. The fix separates the two error cases; only identifiers of differing digit counts are affected today, which is why an in-range invariant probe misses it.

One question I'd like a call on: NewVersion already rejects an out-of-range major/minor/patch, so prerelease is currently the one place that lets it through. Do you want this aligned with that (reject at parse), or kept permissive and just compared numerically? Either is a small change from here — I only picked the minimal one.

@feiiiiii5

Copy link
Copy Markdown
Author

Hi — two small semver fixes are waiting, and no workflow has run on either branch yet:

Both have regression coverage where behaviour changes. Approving the workflows would let CI confirm them; could someone take a look when you have a moment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant