fix: stop signing a package from allocating several times its size - #2170
Open
netomi wants to merge 1 commit into
Open
fix: stop signing a package from allocating several times its size#2170netomi wants to merge 1 commit into
netomi wants to merge 1 commit into
Conversation
Signing streamed the package into BouncyCastle's Ed25519Signer in 1 KiB chunks, which reads as the memory-safe way to do it and is the opposite. Pure Ed25519 hashes the message twice - once for the nonce, once for the challenge - so a signer cannot discard what it is fed, and BouncyCastle's holds it in a ByteArrayOutputStream that doubles its capacity as it fills. The ~300 MB package in #1450 therefore ended up in a 512 MB array with the 256 MB one it was copied from still live: three quarters of a gigabyte to sign 300 MB, which is why that publish needed the heap raised to 2 GB. The reported OutOfMemoryError comes straight out of that growth, at ByteArrayOutputStream.write -> ensureCapacity -> Arrays.copyOf. Read the file into one exactly sized array and sign that instead. Measured on a 9 MiB package: 33,560,752 bytes allocated streaming, 9,445,000 read in one go - 3.6x down to 1.0x. The signature is unchanged. Ed25519Signer does no more than accumulate and hand the message to Ed25519PrivateKeyParameters.sign, which is what this now calls directly, and a test asserts the two produce identical bytes so that signatures already published keep matching. Verification had the same defect the other way round, on the mirroring path, and is fixed the same way. It also now says so when handed a PEM holding something other than an Ed25519 key, rather than letting a ClassCastException out of Ed25519Signer.init. This is a smaller heap, not a bounded one: it still scales with the package, so an instance with the integrity service enabled wants a heap above ovsx.publishing.max-content-size. Constant memory is not reachable through BouncyCastle - every pure-Ed25519 sign overload takes a byte[], and the one streaming signer implements Ed25519ph, a different scheme. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It introduces user-facing and robustness issues (misleading key-read error message, missing signature-length validation, and a potentially flaky allocation-measurement helper in tests) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR changes the extension signature generation and verification flow to avoid excessive heap allocation when signing/verifying large VSIX packages by bypassing BouncyCastle’s streaming Ed25519Signer buffering behavior and using direct Ed25519 byte[] APIs instead.
Changes:
- Switch signing to
Ed25519PrivateKeyParameters.sign(...)over a singlebyte[]message buffer. - Switch verification to
Ed25519PublicKeyParameters.verify(...)over a singlebyte[]message buffer, with improved public-key type diagnostics. - Add regression tests asserting signature equivalence vs the prior streaming approach and measuring allocation behavior.
File summaries
| File | Description |
|---|---|
| server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java | Updates signing/verification to avoid multi-growth buffering and improves key-type error reporting. |
| server/src/test/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityServiceTest.java | Adds equivalence + allocation-regression tests for the updated signing approach. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+91
to
+94
| var message = Files.readAllBytes(extensionFile.getPath()); | ||
| var signature = Files.readAllBytes(signatureFile.getPath()); | ||
| verified = ed25519PublicKey | ||
| .verify(Ed25519.Algorithm.Ed25519, null, message, 0, message.length, signature, 0); |
Comment on lines
+146
to
+152
| private static long allocatedBy(ThrowingRunnable body) throws Exception { | ||
| var threads = (ThreadMXBean) ManagementFactory.getThreadMXBean(); | ||
| var id = Thread.currentThread().threadId(); | ||
| var before = threads.getThreadAllocatedBytes(id); | ||
| body.run(); | ||
| return threads.getThreadAllocatedBytes(id) - before; | ||
| } |
Comment on lines
76
to
78
| } catch (IOException e) { | ||
| throw new ErrorResultException("Failed to read private key file", e); | ||
| } |
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
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.
Towards #1450.
The bug
createSignatureFilestreamed the package into BouncyCastle'sEd25519Signerin 1 KiB chunks. That reads as the memory-safe way to do it and is the opposite of one:Pure Ed25519 (RFC 8032) hashes the message twice — once for the nonce
H(prefix ‖ M), once for the challengeH(R ‖ A ‖ M)— so a signer cannot discard what it is fed. BouncyCastle keeps it in aByteArrayOutputStream, which doubles its capacity as it fills and allocates every intermediate array on the way.The ~300 MB package in #1450 therefore ended up in a 512 MB array with the 256 MB one it had been copied from still live — three quarters of a gigabyte to sign 300 MB. That is why 1 GB of heap failed and 2 GB worked, and the reported stack trace comes straight out of that growth:
ByteArrayOutputStream.write→ensureCapacity→Arrays.copyOf.Worth noting #1458 fixed a different OOM on this path (the
MAX_CONTENT_SIZEcheck). At 304.6 MB the reporter's file is under the 512 MB limit, so it passes that check and dies later, in signing — as they predicted in the thread.The change
Read the file into one exactly sized array and sign that. Measured on a 9 MiB package by the test below:
Ed25519SignerThe signature is unchanged.
Ed25519Signerdoes no more than accumulate the message and hand it toEd25519PrivateKeyParameters.sign, which is what this now calls directly with the same algorithm and a null context.signsExactlyAsTheStreamingSignerDidasserts the two produce identical bytes, so signatures already published keep matching what a re-sign produces.Verification had the same defect the other way round — on the mirroring path, via
MirrorExtensionService— and is fixed the same way. It now also reports a PEM holding something other than an Ed25519 key, instead of letting aClassCastExceptionout ofEd25519Signer.init.What this does not fix
A smaller heap, not a bounded one. It still scales with the package, so an instance with the integrity service enabled wants a heap above
ovsx.publishing.max-content-size(512 MB by default). Constant memory isn't reachable through BouncyCastle: everysignoverload it exposes for pure Ed25519 takes abyte[], and its only streaming signer implements Ed25519ph — a different scheme, whose signatures nothing verifying these packages today would accept. Switching to it, or shelling out to an external signer, is the discussion in #1450 and belongs in its own issue.The silent failure is untouched. #1450's second complaint is that the version is left
active = falsewhile the CLI reports success. That is still true, and there are two reasons for it worth recording:publishAsynccatchesExceptionto callmarkScanAsErrored, butOutOfMemoryErroris anError, so even an instance with scanning enabled records nothing.doPublishcarries@Retryable, and the comment above it assumes retries happen — but it is a private method invoked frompublishAsyncin the same class, so the proxy is bypassed and the annotation never applies. (@EnableResilientMethodsis onRegistryApplication, and the@Retryableon the publiccreateExtensionVersiondoes work.)Both are behavioural changes rather than a memory fix, so they are deliberately not in here.
Tests
signsExactlyAsTheStreamingSignerDid— byte-identical signatures, the guard against a format change.allocatesThePackageOnceWhereStreamingAllocatedItSeveralTimesOver— measures both ways round with per-thread allocation counters, prints the numbers, and asserts the shipped path stays under 1.5× the package while the streaming one exceeds 2×. Thresholds sit well clear of the measured 1.0× and 3.6× so it fails on a regression rather than on JVM noise; it skips where the JVM does not report per-thread allocation.testGenerateSignature, which checks a real.vsixagainst a recorded sigzip fixture, still passes.Full server suite green (1167 tests).
🤖 Generated with Claude Code