Fix version comparison and parsing inconsistencies in maven-artifact - #12942
Open
slachiewicz wants to merge 3 commits into
Open
Fix version comparison and parsing inconsistencies in maven-artifact#12942slachiewicz wants to merge 3 commits into
slachiewicz 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
approved these changes
Aug 30, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
There was a problem hiding this comment.
Clean backport of the version comparison fixes from master (#12947, already reviewed and approved) to maven-4.0.x. The diff is byte-for-byte identical to the master variant — all three fixes apply correctly:
- CombinationItem.compareTo transitivity —
compareTo(null)now correctly delegates todigitPart.compareTo(null)when the string part is a release qualifier, restoring the transitivity contract. - DefaultArtifactVersion hashCode/equals consistency —
orderingHashCode()correctly strips trailing ordering-irrelevant items and normalizes qualifiers. - ComparableVersion max length bound — properly enforced.
The CombinationItem class only exists in 4.x+, so no 3.9.x/3.10.x backport is needed for these fixes. Each fix is independently revertible via its own commit.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.