Repository navigation
Encode form and digest input without CharBuffer - #2385
Open
pavel-ptashyts wants to merge 6 commits into
Open
pavel-ptashyts wants to merge 6 commits into
pavel-ptashyts wants to merge 6 commits into
Conversation
StringUtils.charSequence2ByteBuffer ran charset.encode(CharBuffer.wrap(cs)). The wrapped CharBuffer has no backing array, so the encoder walks it char by char and starts from an estimated buffer size. Every url-encoded form body (HttpUtils.urlEncodeFormParams) and every Digest hash input (Realm) goes through it, on the caller thread. charSequence2Bytes now copies an ASCII StringBuilder straight into a byte array for UTF-8, ISO-8859-1 and US-ASCII, which all map a char below U+0080 to that byte. Any other input or charset goes through String.getBytes(charset), which AsyncHttpClient#2371 showed to produce the same bytes as Charset.encode, replacements and byte order marks included. charSequence2ByteBuffer wraps that array. Signatures are unchanged. New tests check the bytes against Charset.encode for every charset the JDK can encode, for String, StringBuilder and CharBuffer input, and on the form and Digest paths with non-ASCII and unpaired surrogates. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
End-to-end form and Digest timings are dominated by url-encoding and SecureRandom, so the change under test is lost in their noise. Add arms that run only charSequence2ByteBuffer on the builders those paths produce, including a non-ASCII UTF-8 one for the fallback. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AGENTS.md requires ASCII-only repository content. The new test and benchmark strings carried raw non-ASCII characters; write them as Java unicode escapes instead. The strings are unchanged. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
hyperxpro
requested changes
Nov 9, 2026
The only input holding U+0080 also had a later char at or above U+0100, so the whole string took the fallback and a loop that let U+0080 through as a single byte still passed. U+0080 on its own and between ASCII chars now pin the boundary of the ASCII copy. StringBodyEncodeBenchmark.charsetEncode stood for the old Charset.encode path by calling charSequence2ByteBuffer with a String, which now goes to String.getBytes as well. It calls UTF_8.encode(CharBuffer.wrap(body)) directly so it keeps measuring the old path. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ISO-8859-1 is the default Digest charset, and the Digest hash input and the Authorization header were taking the String copy fallback for any char from U+0080 to U+00FF, such as an accented letter in the user name or realm. ISO-8859-1 maps every char below U+0100 to that byte, so the copy loop now runs up to U+0100 for it and up to U+0080 for UTF-8 and US-ASCII, which leaves the fallback to input that has chars ISO-8859-1 cannot map. StringUtilsTest already holds U+00FF and U+0100 next to each other for every charset. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
HA1 is a hex digest and always ASCII; what digestUtf8 makes non-ASCII is the user:realm:password input hashed into it, which is the input that takes the String copy fallback. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Summary
StringUtils.charSequence2Bytes/charSequence2ByteBufferno longer runcharset.encode(CharBuffer.wrap(cs)). They are on the path of every url-encoded form body (HttpUtils.urlEncodeFormParams) and every Digest hash (Realm), and of the DigestAuthorizationheader (AuthenticatorUtils).setBody(String).Problem
CharBuffer.wrap(CharSequence)returns a buffer with no backing array, so the JDK encoder walks it char by char through virtualget/putcalls and misses the array loop and theStringintrinsics. All three callers pass a pooledStringBuilder, on the caller thread, once per request attempt (form) or three times per Digest response.Change
charSequence2Bytes:Stringgoes throughString.getBytes(charset);StringBuilder(or any otherCharSequence) in UTF-8, ISO-8859-1 or US-ASCII is copied char by char into an exact-sizebyte[]while every char is below a limit under which the charset maps a char to that byte: U+0100 for ISO-8859-1, U+0080 for UTF-8 and US-ASCII. On the first char at or above the limit it falls back totoString().getBytes(charset);toString().getBytes(charset).charSequence2ByteBufferwraps that array. A url-encoded form is always ASCII (everything else is percent-encoded), so form bodies never reach the fallback. The Digest hash input (Realm) and the DigestAuthorizationheader (AuthenticatorUtils.computeRealmHeader) reach it in UTF-8 (charset=UTF-8) when the credentials, realm or uri hold a char at or above U+0080, and in ISO-8859-1, the default Digest charset, only when they hold a char ISO-8859-1 cannot map (at or above U+0100), so Latin-1 credentials such as accented letters stay on the loop.StringBodyEncodeBenchmark.charsetEncodefrom #2371 measured the old path throughcharSequence2ByteBuffer(String, UTF_8), which now goes togetBytestoo; it now callsUTF_8.encode(CharBuffer.wrap(body))directly, so it keeps measuring the old path.String.getBytes(charset)andCharset.encodeboth replace malformed and unmappable input with the encoder's replacement and write the same byte order marks; #2371 checked this for every charset. The new tests check it again here:StringUtilsTest: both methods againstcharset.encode(CharBuffer.wrap(cs))for every charset the JDK can encode, 17 edge inputs (unpaired surrogates, supplementary chars, BOM, replacement char, NUL/DEL/U+0080/U+00FF/U+0100, and U+0080 alone and between ASCII chars, so each limit is checked on its own: with> 0x80, or with the ISO-8859-1 limit at 0x101, the test fails), and four kinds ofCharSequence:String,StringBuilder, aStringBuilderwith a UTF-16 coder (the state a pooled builder keeps after it once held a char above U+00FF) andCharBuffer.HttpUtilsTest:urlEncodeFormParamsagainstUS_ASCII.encodeof the same url-encoded form, in UTF-8, ISO-8859-1 and GBK, with non-ASCII, a supplementary char, unpaired surrogates and a null value, from a pooled builder with a UTF-16 coder.RealmTest: the Digest response for Cyrillic user, a password with a lone surrogate, Euro sign and emoji, and a non-ASCII realm, in ISO-8859-1 (default) and UTF-8 (charset=UTF-8), against MD5 overcharset.encodebytes.The returned
ByteBuffernow has capacity equal to its limit. Before, it was whatever the encoder had allocated (1.1 bytes per char for UTF-8, more after growth). That capacity is not specified byCharset.encode, and every caller reads only position to limit.An alternative,
sb.toString().getBytes(charset)for everything, is faster still but copies the builder into aStringfirst, which roughly doubles allocation per form body against main (numbers below), so it was not taken.Benchmark
FormDigestEncodeBenchmark(new) calls production code only, so the same class runs against main and this branch. JDK 17,-f 2 -wi 4 -i 5 -prof gc, three rounds alternating main and branch; the median round is shown.encodeFormBuilder: 6,691-byte url-encoded form, US-ASCIIencodeDigestBuilder: 131-char ASCII Digest input, ISO-8859-1encodeUtf8DigestBuilder: Cyrillicuser:realm:password, UTF-8formSmall:urlEncodeFormParams, 4 paramsformLarge:urlEncodeFormParams, 256 paramsdigestAscii:Realm.Builder.build(), ASCII credentialsdigestUtf8:Realm.Builder.build(), Cyrillic credentials, UTF-8The end-to-end form and Digest timings are dominated by url-encoding and by
SecureRandomfor the cnonce. On this shared host they moved by more than the change itself between runs of the same build (maindigestAsciimeasured anywhere from 1.8 to 9.2 us), so only their B/op is reported. B/op is deterministic up to a few dozen bytes.encodeUtf8DigestBuilderallocates more: the loop's array is discarded at the first non-ASCII char, andgetBytes(UTF_8)on a UTF-16 string uses a 3-bytes-per-char scratch array, the same trade-off as in #2371. In ISO-8859-1, the default Digest charset, the loop covers every char up to U+00FF, so the fallback, with its discarded array andStringcopy, is left to credentials, realm or uri holding a char ISO-8859-1 cannot map (it becomes?, as before). Either way this is short Digest input only; ASCII Digest input and every form body stay on the loop. The ISO-8859-1 limit was raised from U+0080 to U+0100 after review; the numbers above were measured before that and do not touch it (encodeDigestBuilderis ASCII).Loop against
toString().getBytes(exploratory, not committed), the encoding step alone, UTF-8,-f 2 -wi 4 -i 5 -prof gc, ns/op and B/op:Charset.encodetoString().getBytesThe host was shared with other builds, so take the timings as indicative.
Second-agent review
Both reviewers ran read-only, in fresh sessions, on
git diff upstream/main...HEAD.AGENTS.md: Codexgpt-6-astra, reasoning effort medium.claude-opus-5-5, effort medium, tools Read/Grep/Glob only.46ff4250d665746074(Java unicode escapes).665746074.665746074Charset.encode's result is unspecified and every caller reads only position to limit (see Change).toString().getBytes, including a UTF-16-coded builder. Measured (table above); the loop is kept for its lower allocation.665746074, with the author's answers to rounds 1 and 2a763e6bc9, after the maintainer review below253d17b3c(limit U+0100 for ISO-8859-1). (b) The description left outAuthenticatorUtilsamong the paths that reach the fallback. Fixed in this description.253d17b3cdigestUtf8bench Javadoc said HA1 is not ASCII; it is the HA1 input. Fixed in9dd49bd8b.9dd49bd8bMaintainer review on
665746074, addressed ina763e6bc9and253d17b3c:c > 0x80passed every test: U+0080 alone andaU+0080bwere added toStringUtilsTest; the mutant now fails.StringBodyEncodeBenchmark.charsetEncodemeasuredgetBytesthroughcharSequence2ByteBuffer: it now callsUTF_8.encode(CharBuffer.wrap(body)).AI disclosure
Claude Code on behalf of Pavel Ptashyts. The commits include
Co-Authored-ByperAGENTS.md.Test plan
StringUtilsTest,HttpUtilsTest,RealmTest,AuthenticatorUtilsTest,DigestAuthTest,DigestAuthRfc7616Test,ParamEncodingTest: 94 tests pass.mvnw.cmd -B -ntp -Dgpg.skip=true clean verifyon JDK 17 (no JDK 11 on the host): 1,998 tests, 0 failures, 0 errors, 35 skipped, on665746074and again on253d17b3c; Revapi passed.9dd49bd8bchanges only a benchmark Javadoc (JMH sources are not built by Maven). Source/target is 11. JDK 11 coverage comes from CI.c > 0x80and an ISO-8859-1 limit of 0x101 each makeStringUtilsTestfail.StringBodyEncodeBenchmarkandFormDigestEncodeBenchmarkcompile withjavac --release 11.Claude Code on behalf of Pavel Ptashyts
🤖 Generated with Claude Code