Path memory sharing + 32 max parts enforcement#2156
Conversation
Owned paths were a separate String per copy, and the origin announce fan-out copies the same path many times: the trie leaf, the cleanup task, one pending-map key per consumer, then every session-layer map downstream. That multiplies to O(broadcasts x consumers) heap strings on a relay. Path's owned representation is now a suffix of a shared Arc<str> (buffer + start offset). Cloning bumps a refcount, and strip_prefix / next_part are offset arithmetic, so a single publish allocates the path once and every consumer shares it. The public API is unchanged; comparisons, ordering, and hashing still go through as_str(). Measured with 10k broadcasts and 50 announce consumers: live heap drops from 70.7 MiB to 52.1 MiB and total allocations from 147.9 MiB to 110.6 MiB. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Walkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rs/moq-net/src/path.rs (1)
210-252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd rustdoc to the changed public methods.
as_str,empty,is_empty,len,to_owned, andinto_ownedarepubbut lack doc comments (unlikeborrowjust below). This region also documents important sharing semantics worth stating explicitly (e.g.to_owned/into_ownedpreserve the sharedArcallocation without copying).As per coding guidelines: "every
pubRust item ... must have a doc comment."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-net/src/path.rs` around lines 210 - 252, Add rustdoc comments to the public methods as_str, empty, is_empty, len, to_owned, and into_owned, describing their return values and behavior. Document the ownership and sharing semantics explicitly, including that to_owned and into_owned preserve existing shared Arc allocations without copying.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rs/moq-net/src/path.rs`:
- Around line 210-252: Add rustdoc comments to the public methods as_str, empty,
is_empty, len, to_owned, and into_owned, describing their return values and
behavior. Document the ownership and sharing semantics explicitly, including
that to_owned and into_owned preserve existing shared Arc allocations without
copying.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87df4992-5f84-484e-8d9b-a1713d3007c9
📒 Files selected for processing (1)
rs/moq-net/src/path.rs
moq-transport caps namespace tuples at 32 fields but moq-lite paths had no depth limit, so a peer could announce arbitrarily deep paths and grow the origin tree without bound. Path::MAX_PARTS (32) is now shared by both protocols. The lite Encode/Decode impls reject deeper paths, the ietf namespace codec reuses the constant, and publish_broadcast refuses a joined path past the limit since a decoded prefix and suffix are each within bounds but their join may not be. Path::parts() is a new public iterator over the slash-separated parts. js/net mirrors the same rules: Path.MAX_PARTS, Path.parts, and a Path.decode validator used at every lite wire read. The ietf namespace decoder previously looped an attacker-controlled count with no bound at all; it now rejects before reading the parts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kixelated
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
js/net/src/ietf/namespace.ts (1)
5-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the updated exported codec APIs.
encodeanddecodeare exported but have no doc comments describing their IETF namespace wire contract and bound-failure behavior.Also applies to: 18-27
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/net/src/ietf/namespace.ts` around lines 5 - 16, The exported encode and decode functions need documentation describing the IETF namespace wire format and their behavior when the namespace exceeds Path.MAX_PARTS. Add doc comments directly above both functions, referencing their exported API contract and documenting the bound-related error behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/net/src/path.ts`:
- Around line 41-46: from can brand paths exceeding MAX_PARTS as Valid, allowing
Lite serialization to emit paths rejected by decoding. Add a shared outbound
path validator that checks the component count against MAX_PARTS, then invoke it
before serializing every Lite path field, using the existing path utilities and
Lite encoder symbols to keep validation consistent.
---
Nitpick comments:
In `@js/net/src/ietf/namespace.ts`:
- Around line 5-16: The exported encode and decode functions need documentation
describing the IETF namespace wire format and their behavior when the namespace
exceeds Path.MAX_PARTS. Add doc comments directly above both functions,
referencing their exported API contract and documenting the bound-related error
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26a8e188-60ea-4d7a-8f83-a3bc0e196129
📒 Files selected for processing (10)
js/net/src/ietf/namespace.tsjs/net/src/lite/announce.tsjs/net/src/lite/fetch.tsjs/net/src/lite/subscribe.tsjs/net/src/lite/track.tsjs/net/src/path.test.tsjs/net/src/path.tsrs/moq-net/src/ietf/namespace.rsrs/moq-net/src/model/origin.rsrs/moq-net/src/path.rs
Decode-side enforcement alone let a local peer emit a path the remote is required to reject. Path.encode validates before every lite path write, mirroring the Rust Encode impl. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
js/net/src/lite/announce.ts (1)
169-175: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the suffixes count in
AnnounceInit.#decodebefore the loop.The
countread from the wire is unbounded. A malicious peer can send an enormous count, causing the decoder to attempt reading millions of strings before anyPath.decodecall rejects. The IETF namespace decoder injs/net/src/ietf/namespace.tsalready boundscount > Path.MAX_PARTSbefore its loop — consider applying a similar guard here for consistency with the PR's theme of bounding attacker-controlled counts.This is pre-existing (the count read was not changed in this PR), but it's low-effort to add and aligns with the PR's defensive scope.
♻️ Suggested bound check
static async `#decode`(r: Reader): Promise<AnnounceInit> { const count = await r.u53(); + if (count > Path.MAX_PARTS) { + throw new Error(`announce init suffixes exceed ${Path.MAX_PARTS}`); + } const suffixes: Path.Valid[] = []; for (let i = 0; i < count; i++) { suffixes.push(Path.decode(await r.string())); } return new AnnounceInit(suffixes); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/net/src/lite/announce.ts` around lines 169 - 175, Bound the attacker-controlled count in AnnounceInit.#decode before iterating. After reading count and before the suffixes loop, reject or throw when count exceeds Path.MAX_PARTS, matching the guard used by the IETF namespace decoder, while preserving normal decoding for valid counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@js/net/src/lite/announce.ts`:
- Around line 169-175: Bound the attacker-controlled count in
AnnounceInit.#decode before iterating. After reading count and before the
suffixes loop, reject or throw when count exceeds Path.MAX_PARTS, matching the
guard used by the IETF namespace decoder, while preserving normal decoding for
valid counts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9ff8a7a-23da-4466-a07b-1797d0535d5e
📒 Files selected for processing (6)
js/net/src/lite/announce.tsjs/net/src/lite/fetch.tsjs/net/src/lite/subscribe.tsjs/net/src/lite/track.tsjs/net/src/path.test.tsjs/net/src/path.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- js/net/src/lite/track.ts
- js/net/src/path.ts
1 participant
Summary
Two changes to
Pathaimed atOriginProducer/OriginConsumermemory usage and bounds.1. Share one allocation across owned Path copies
Every owned path was its own
String, and the origin announce fan-out copies the same path many times: the trie leaf (OriginBroadcast.path), the cleanup task closure, one pending-map key per consumer, and then every session-layer map downstream (stats_guards, subscriberproducers, ietfnamespaces, stats entries). That multiplies to O(broadcasts x consumers) heap strings on a relay.Path's owned representation is now a suffix of a sharedArc<str>(buffer + start offset):clone()andto_owned()on an owned path bump a refcount instead of copying.strip_prefix/next_part(the per-consumer root strip in announce fan-out) are offset arithmetic on the shared buffer.publish_broadcastallocates the full path once; everything downstream shares it. No changes were needed outsidepath.rs.Measured with 10k broadcasts x 50 announce consumers (counting allocator, release build):
Rejected alternatives:
Vec<Arc<String>>segments (loses contiguousas_str()for wire/Ord/Hash; per-part Arc overhead; theOriginNodetrie already interns segments structurally), global interner (needs GC for churning broadcasts; the dominant waste was same-value copies), tinyvec/array (does not remove the string allocations).2. Enforce 32 max path parts in moq-lite, matching moq-transport
moq-transport caps namespace tuples at 32 fields; moq-lite paths had no depth limit, so a peer could announce arbitrarily deep paths and grow the origin tree without bound.
Path::MAX_PARTS(32) is now shared by both protocols;Path::parts()is a new public iterator.Encode/Decodeimpls reject deeper paths (covers announce/subscribe/fetch/setup uniformly).publish_broadcastrefuses a joined path past the limit: a decoded announce prefix and suffix are each within the wire bound, but their join may not be. This also bounds trie depth and guarantees forwarded paths can re-encode.Path.MAX_PARTS,Path.parts, and aPath.decodevalidator used at every lite wire read site. The JS ietf namespace decoder previously looped an attacker-controlled count with no bound at all; it now rejects before reading parts.Branch-targeting note: the lite depth limit rejects wire input that was previously accepted. The ietf side already enforced it (spec compliance), and this reads as DoS hardening, so it targets
mainper "when in doubt"; redirect todevif you consider it a lite contract change. The moq-lite draft should probably document the limit as well (not part of this PR).Public API changes
Path::MAX_PARTS(new pub const) andPath::parts()(new method): additive.Pathinternals reshaped (field was already private); all existing methods keep their signatures. DerivedPartialEq/Eq/Ord/Hash/Debug/Serializereplaced with manual impls that go throughas_str(), matching the previousCowforwarding behavior.Path.MAX_PARTS,Path.parts,Path.decode(all additive).publish_broadcastreturns false for them.Test plan
cargo test -p moq-net(383 tests) + doc tests; new tests pin Arc sharing via pointer equality, wire limit round-trip at/past 32 parts, and publish rejection (including root+path join)bun testjs/net (161 tests) incl. newparts/decodetests;tsc -band biome cleancargo check --workspace, clippy clean,RUSTDOCFLAGS=-D warnings cargo doc(Written by Claude Fable 5)
🤖 Generated with Claude Code