Fix version comparison and parsing inconsistencies in maven-artifact - #12947
Fix version comparison and parsing inconsistencies in maven-artifact#12947slachiewicz wants to merge 3 commits into
Conversation
Every '-' in a version string nests another list item, so comparison,
equality, hash code and canonicalization recurse one frame per level;
very long, deeply hyphenated version strings can exhaust the stack, and
very long digit runs cost quadratic time to parse into BigInteger.
Cap parseVersion's input at 256 characters, far beyond any real-world
version identifier, so all three costs stay bounded in one place.
Measurements behind the 256-character bound (this platform: macOS/
aarch64, one representative JDK; plausibly JDK- and platform-dependent,
worth re-checking on the Linux CI JVM before treating these thresholds
as authoritative elsewhere):
- JIT-warmed, deep-vs-deep hashCode/equals/getCanonical overflow
around 500 nested levels at a 256-500k thread stack size. (A
comparison against a short, non-nested version such as "1" is not
representative: it returns early on the first mismatched item type
and never recurses, so it understates the risk.)
- Cold, interpreter-only execution (-Xint, one shot per data point,
representative of a freshly started JVM rather than a warmed-up
long-running one, which matches how this code is normally invoked)
against the smallest thread stack size the JVM will start at all on
this platform (208k) overflows at as few as ~240 nested levels -
below the ~256 levels a 512-character cap would allow. That
combination is realistic enough (Maven runs as a fresh process, and
embedders can and do configure small thread stacks) that a
512-character cap could not be called clearly safe.
- The same cold/interpreted measurement at a 228k stack overflows
around 320-330 levels, giving the 256-character cap (~128 levels)
roughly 2.4x headroom there, and ~1.8x headroom even at the 208k
floor.
No compatibility cost identified: nothing in this repository's own
source, tests or POMs uses a version string longer than 40 characters,
and known long-form real-world version schemes (git-describe output,
Debian native versions, Eclipse OSGi qualifiers, timestamp-based
builds) stay well under 100.
DefaultArtifactVersion.equals is defined as compareTo() == 0 (so, for example, "1-ga" equals "1"), but its hashCode delegated to ComparableVersion's structural hash, which does not agree: "1-ga" and "1" parse to different item trees and therefore hash differently, violating the equals/hashCode contract and breaking hash-based collections built over repository version lists. Add ComparableVersion.orderingHashCode(), a hash derived from the same comparison rules compareTo() already uses (trailing null-equivalent items dropped, qualifiers normalized), and have DefaultArtifactVersion use it instead of the structural hash.
… version
CombinationItem.compareTo(null) only consulted the string part, so a
release-equivalent qualifier ("ga", "final", "release") always compared
equal to null regardless of any trailing digit: "1-ga1" compared equal
to "1", and to "1-ga2" as well, while "1-ga1" itself sorts strictly
before "1-ga2". That breaks compareTo's transitivity contract and lets
a range or exact-version restriction match a differently-spelled
version it did not intend to: an exact pin [1.2.3] matched "1.2.3-ga2".
When the string part is release-equivalent, fall through to the digit
part so "1-ga1" sorts strictly after "1", matching the ordering the
surrounding comment already documents. Plain qualifiers with no digit
("1-ga") and a zero digit ("1-ga0") keep comparing equal to "1", as
documented elsewhere in this class.
Includes a range-level regression test demonstrating the exact-pin
scenario directly, alongside the ComparableVersion-level ordering test,
so a future cherry-pick of this fix cannot silently drop the coverage
that guards it.
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
All three fixes are correct, well-reasoned, and backed by comprehensive tests. High-quality contribution addressing genuine Comparable contract violations in core Maven infrastructure.
Fix-by-fix assessment:
-
compareTo transitivity — Correct. Only changes behavior when the string part is release-equivalent AND a non-zero digit follows, matching the documented intent (
1-rc1 < 1,1-ga1 > 1). The exact-version range test ([1.2.3]no longer matches1.2.3-ga2) is a valuable regression guard. -
hashCode/equals consistency —
orderingHashCode()correctly mirrorscompareTosemantics: trailing null-equivalent items stripped before hashing, release-equivalent qualifiers hash identically viacomparableQualifier(),CombinationItemcombines qualifier and digit hashes. Traced through multiple cases (1vs1-ga,1-ga1vs1-final1,1-SNAPSHOTvs1.0-SNAPSHOT) — all produce consistent hashes. -
MAX_VERSION_LENGTH bound — Well-justified (commit message provides stack depth measurements).
IllegalArgumentExceptionis appropriate — failing fast with a clear message beats stack overflow or quadratic BigInteger parsing.
Pre-existing issue (not introduced by this PR):
CombinationItem.compareTo(StringItem) at ~line 466 unconditionally returns 1 when string parts are equal, without consulting the digit part. This means 1-ga0 > 1-ga even though both compare equal to 1 (same transitivity violation class as fix #1). Consider extending the fix to this case — or a follow-up PR.
📋 PR Metadata
| Aspect | Current | Suggested |
|---|---|---|
| Milestone | (none) | 4.1.0 |
🔀 Backport Status
maven-4.0.x (same CombinationItem code). Fixes #2 and #3 also affect maven-3.9.x and maven-3.10.x but would require adapted implementations since CombinationItem does not exist on those branches.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
elharo
left a comment
elharo
left a comment
There was a problem hiding this comment.
If we impose a length restriction on version qualifiers we need to document that on the site, though I don't think you can make those changes in this PR since the repos are different. However, that PR should be ready to go.
|
|
||
| /** | ||
| * Maximum accepted length of a version string. Version strings routinely come from external | ||
| * repository metadata; without a bound, every {@code -} separator nests another list whose |
There was a problem hiding this comment.
metadata; without --> metadata. Without
| /** | ||
| * Maximum accepted length of a version string. Version strings routinely come from external | ||
| * repository metadata; without a bound, every {@code -} separator nests another list whose | ||
| * comparison, equality, hash code and canonicalization recurse one frame per level, and digit |
| * at quadratic cost. 256 characters is far beyond any real-world version identifier while | ||
| * keeping the nesting depth (at most about half the length) and numeric items small. | ||
| */ | ||
| private static final int MAX_VERSION_LENGTH = 256; |
There was a problem hiding this comment.
I don't know. I'd probably take a 50-50 bet that there are longer version strings out there. Would it be feasible to go to 1024? or higher? At what length do we actually have problems?
| return stringPart.compareTo(item); | ||
| int result = stringPart.compareTo(item); | ||
| if (result == 0) { | ||
| // the string part is equivalent to the release qualifier ("ga", "final", "release"), |
There was a problem hiding this comment.
have we tested 1-ga2 vs 1-ga11?
| } | ||
| yield hash; | ||
| } | ||
| // qualifiers that compare as equal ("ga", "final", "release" and the empty qualifier) must hash alike |
3 participants
Three independent fixes in
maven-artifact's version handling.1-ga1compared equal to1, while1-ga1 < 1-ga2. That breaks the transitivity contractComparablerequires, and contradicts the intent stated in the code's own comment (1-rc1 < 1,1-ga1 > 1).CombinationItem.compareTo(null)consulted only the string part and never the digit. Includes aVersionRange-level regression test: an exact range[1.2.3]no longer matches1.2.3-ga2.hashCode/equalsconsistency.DefaultArtifactVersion.equalsis defined viacompareTo, buthashCodewas structural, so two order-equal versions could hash differently.HashSetandTreeSetbuilt from the same pair disagreed on size.equalsandcompareToare unchanged, so no resolution outcome moves.ComparableVersionnow rejects version strings longer than 256 characters. Nested-separators recurse per level and long digit runs cost quadratic time inBigInteger; on a reduced worker-thread stack the cold overflow floor is a few hundred levels. Nothing in this repository has a version string over 40 characters. Measurements are in the commit body.Each fix is a separate commit and independently revertible.