-
Notifications
You must be signed in to change notification settings - Fork 52
Bound decoder work to prevent a pointer fan-out DoS (STF-1571) #442
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
80e3bea
56300ff
7623b3c
3eadcb5
3884fa5
2eda46c
ee6d341
d78374f
d448fb1
0f74740
e23b67b
620924d
74ccdb5
92f710d
d1e6dc4
140b5a7
d87f0a5
791a050
f314068
553be73
a0ed65e
0731430
715e999
e0fa785
a326219
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,5 @@ | ||
| package com.maxmind.db; | ||
|
|
||
| import java.nio.charset.CharacterCodingException; | ||
| import java.nio.charset.CharsetDecoder; | ||
|
|
||
| /** | ||
| * A generic buffer abstraction that supports sequential and random access | ||
| * to binary data. Implementations may be backed by a single {@link | ||
|
|
@@ -96,13 +93,4 @@ sealed interface Buffer permits SingleBuffer, MultiBuffer { | |
| * @return a duplicate buffer | ||
| */ | ||
| Buffer duplicate(); | ||
|
|
||
| /** | ||
| * Decodes the buffer's content into a string using the given decoder. | ||
| * | ||
| * @param decoder the charset decoder | ||
| * @return the decoded string | ||
| * @throws CharacterCodingException if decoding fails | ||
| */ | ||
| String decode(CharsetDecoder decoder) throws CharacterCodingException; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Flagging on this file generally rather than a specific line: Worth removing both. 🤖 Comment by Claude (Claude Code) on behalf of Will.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are leaving Codex, on Greg’s behalf. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,42 @@ | ||
| package com.maxmind.db; | ||
|
|
||
| /** | ||
| * {@code DecodedValue} is a wrapper for the decoded value. | ||
| * An opaque decoded value and its resource costs, produced by {@link NodeCache.Loader}. | ||
| * Caches retain this instance unchanged for its original key. See {@link NodeCache}. | ||
| */ | ||
| public final class DecodedValue { | ||
| private static final int PAYLOAD_SHIFT = 8; | ||
| private static final int VALUES_SHIFT = 30; | ||
| private static final long PAYLOAD_MASK = (1L << 22) - 1; | ||
|
|
||
| // Final fields preserve their initialized values when a cache publishes this object. | ||
| final Object value; | ||
| private final long costs; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reasoning about racy publication is sound, but nothing exercises it. Not deterministically testable, but e.g. 256 threads against 🤖 Comment by Claude (Claude Code) on behalf of Will.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a four-worker test using one Codex, on Greg’s behalf. |
||
|
|
||
| DecodedValue(Object value) { | ||
| DecodedValue(Object value, int values, long payloadBytes, int depth) { | ||
| this.value = value; | ||
| this.costs = ((long) values << VALUES_SHIFT) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The cost packing has zero headroom, no guard, and nothing ties it to The constructor masks and validates nothing. Field widths live here; the limits they encode live in The depth case fails in the unsafe direction: a cached target recording depth cost 0 passes Suggest range-checking (or asserting) the three arguments here, and deriving 🤖 Comment by Claude (Claude Code) on behalf of Will.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kept the packed representation and tied its round-trip tests to the actual Codex, on Greg’s behalf. |
||
| | (payloadBytes << PAYLOAD_SHIFT) | ||
| | depth; | ||
| } | ||
|
|
||
| Object value() { | ||
| return value; | ||
| } | ||
|
|
||
| static int values(long costs) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These read as absolute counts but every one returns a delta (the cost of one target, as computed by Broader point on the packing itself: 🤖 Comment by Claude (Claude Code) on behalf of Will.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the redundant instance accessors, but kept the Codex, on Greg’s behalf. |
||
| return (int) (costs >>> VALUES_SHIFT); | ||
| } | ||
|
|
||
| static long payloadBytes(long costs) { | ||
| return (costs >>> PAYLOAD_SHIFT) & PAYLOAD_MASK; | ||
| } | ||
|
|
||
| static int depth(long costs) { | ||
| return (int) (costs & 0xFF); | ||
| } | ||
|
|
||
| long costs() { | ||
| return this.costs; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pointer-to-pointer rejection is a separate behavior change, not a resource limit — a custom database that decoded in 4.1.0 will now throw — and it appears only in
UPGRADING.md. Worth its own bullet here: "The decoder now rejects a data-section pointer whose target is another pointer, which the MaxMind DB format does not permit."Same for the collection capacity-hint change.
Everything else in this section checks out — I verified each exception-type claim against a real check (control byte, extended type byte, size header, pointer,
double,float), and the version bump to 4.2.0 is right for a release adding behavior changes.🤖 Comment by Claude (Claude Code) on behalf of Will.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kept pointer-to-pointer validation and capacity-hint details in
UPGRADING.md, with an explicit changelog link to those changes. Consolidated overlapping release-note entries instead of adding more implementation details to the changelog.Codex, on Greg’s behalf.