discovery: bound channel range reply memory usage - #10992
Conversation
65e517e to
551bdec
Compare
|
/gateway review |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gateway review — 4 findings
🔴 0 Blocker · 🟠 2 Major · 🟡 2 Minor · 🔵 0 Nit
Summary
This PR replaces a coarse 67MB zlib byte-cap with a precise 100k short-channel-ID count cap, adds an aggregate per-query SCID budget, resets accumulated reply state on any error, and counts the reply budget from each reply's own encoding. The intent — making the channel-range-sync working set predictable against a hostile peer — is sound, and the memory bound itself is now correctly enforced (buffered ≤ received ≤ maxChanRangeReplySCIDs), with good test coverage.
Two concerns should be resolved before merge. First, processChanRangeReply indexes msg.Timestamps[i] over msg.ShortChanIDs without establishing that the two slices are the same length; if the lnwire.ReplyChannelRange decoder does not enforce parity, a crafted reply panics the gossip goroutine — a DoS on the very path this PR hardens. Second, the zlib decoder now rejects io.ErrUnexpectedEOF, which the code being replaced (last touched deliberately in PR #4391) accepted as a clean terminator; this needs confirmation that no conforming peer relies on the old behavior. Two smaller issues round out the review.
Bot commands
/gateway re-review— re-run after pushing changes/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
| g.numChanRangeReplySCIDsRcvd += numReplySCIDs | ||
| g.prevReplyChannelRange = msg | ||
|
|
||
| for i, scid := range msg.ShortChanIDs { |
There was a problem hiding this comment.
🟠 F1 (Major) — Mismatched Timestamps length can panic the gossip syncer
A remote peer can crash the gossip-syncer goroutine with a ReplyChannelRange whose Timestamps field is non-empty but shorter than ShortChanIDs: the buffer loop does msg.Timestamps[i] for i ranging over msg.ShortChanIDs whenever len(msg.Timestamps) != 0, with no check that the two lengths match. That is an index-out-of-range panic driven entirely by untrusted peer input.
Why this matters
msg is peer-supplied and Timestamps is a length-delimited wire field decoded independently of ShortChanIDs, so the parity len(msg.Timestamps) == len(msg.ShortChanIDs) must be guaranteed by lnwire.ReplyChannelRange.Decode — which is not in the provided context, so I cannot confirm it. If the decoder does not reject the mismatch, this is a blocker (reachable panic on a hostile path); it is reported as major pending that confirmation. The loop predates this PR, but the enclosing block is being relocated here and this PR is specifically about bounding hostile channel-range input, so it is the right place to close it: either add if len(msg.Timestamps) != 0 && len(msg.Timestamps) != len(msg.ShortChanIDs) { return fmt.Errorf(...) } before the loop, or confirm the decoder enforces parity.
There was a problem hiding this comment.
This is already checked during wire parsing and in several other layers.
| var cid ShortChannelID | ||
| err := ReadElements(r, &cid) | ||
|
|
||
| switch { |
There was a problem hiding this comment.
🟠 F2 (Major) — Zlib decoder now rejects io.ErrUnexpectedEOF that #4391 accepted
decodeCompressedShortChanIDs treats only io.EOF as clean stream termination, whereas the loop it replaces treated both io.EOF and io.ErrUnexpectedEOF as clean and returned the SCIDs read so far. Any zlib reply whose final ReadElements returns io.ErrUnexpectedEOF is now rejected outright and terminates the range sync, where it was previously accepted.
Why this matters
Blame shows the EOF/ErrUnexpectedEOF handling was last written deliberately by matheusd in PR #4391 (2020), on top of Roasbeef's original #1399 (2018) — precisely the branch this diff drops. There is a strong argument this is correct hardening rather than a regression: a BOLT-conforming zlib stream decodes to a whole number of 8-byte SCIDs, and the old ErrUnexpectedEOF acceptance was tied to the now-removed io.LimitedReader, which could truncate a valid stream mid-element at maxZlibBufSize. With the LimitedReader gone, ErrUnexpectedEOF should only arise from genuinely malformed/truncated input, which is correct to reject. The remaining question is empirical: confirm no real-world peer implementation emits a stream that decodes with a trailing partial element under normal conditions before landing. Reach is low (lnwire decode paths plus processChanRangeReply), so the blast radius is a per-peer sync break, not a local crash. Restore the io.ErrUnexpectedEOF case if the historical behavior turns out to be load-bearing.
| } | ||
|
|
||
| var replyCount uint32 | ||
| switch msg.EncodingType { |
There was a problem hiding this comment.
🟡 F3 (Minor) — Reply-message budget keyed on peer-supplied encoding type
The reply-count weight now derives from msg.EncodingType (peer-controlled) instead of g.cfg.encodingType (what we requested). A peer answering with EncodingSortedPlain gets replyCount = 1 and can send up to maxReplies (500) ReplyChannelRange messages, versus the ~125 that the maxQueryChanRangeRepliesZlibFactor = 4 weighting enforced when zlib was in play — a ~4x amplification of accepted messages and their per-message validation/buffering passes. Total buffered memory is still hard-bounded by the independent maxChanRangeReplySCIDs = 100_000 cap, so this is message-count amplification, not a memory blow-up. Consider keying replyCount off the requested encoding, or counting the reply budget in SCIDs rather than messages.
| // compute the number of bytes encoded based on the size of the | ||
| // query body. | ||
| numShortChanIDs := len(queryBody) / 8 | ||
| if numShortChanIDs > maxDecodedShortChanIDs { |
There was a problem hiding this comment.
🟡 F4 (Minor) — maxDecodedShortChanIDs comment and plain-path guard mislead
The maxDecodedShortChanIDs comment describes it as the max IDs "decoded from a single compressed message" that "bounds decompression," but the guard is also applied to the uncompressed EncodingSortedPlain branch, and on that branch it is effectively unreachable: numShortChanIDs = len(queryBody)/8 is bounded by the ~64KB wire message to roughly 8191, far below the 100_000 cap. The real cross-message plain-encoding bound lives in syncer.go via numChanRangeReplySCIDsRcvd. Reword the comment to cover both encodings and note that the per-message plain guard is defense-in-depth, so a reader doesn't mistake it for where the limit actually bites.
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
In this commit, we cap each decompressed short channel ID set at 100,000 entries, matching the aggregate range reply budget. The old zlib reader bounded compressed input rather than decoded output, so the two working-set limits could drift apart. We retain compatibility with protocol-valid compressed replies, reject truncated or corrupt zlib streams, and close the reader on every exit. Boundary, compatibility, corruption, and property tests cover the decoder.
In this commit, we cap each QueryChannelRange response at 100,000 SCIDs across all streamed replies. The existing reply-count limit did not track the aggregate decoded working set, so memory use varied with the encoding and composition of the reply stream. We count raw SCIDs before timestamp filtering, charge replies using the received encoding type, and release all accumulated range state on any error. This bounds both memory and CPU work while still leaving headroom above the current graph.
551bdec to
ce56fef
Compare
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 3 findings
🔴 0 Blocker · 🟠 2 Major · 🟡 1 Minor · 🔵 0 Nit
Summary
This PR bounds the working set during channel-range sync, and the memory-bounding logic itself is sound: the new per-message and per-query SCID caps are enforced incrementally, the error-path resetChanRangeReplyState() correctly releases accumulated state, and the aggregate-count check is free of integer underflow. One prior finding (F4) was cleanly addressed by rewording the maxDecodedShortChanIDs comment.
Three prior findings remain unresolved. The most important is F1: the peer-controlled msg.Timestamps[i] access in the buffering loop is still guarded only by a non-empty check, not a length-parity check — three independent specialists flagged this as a remote-reachable panic. The maintainer's inline reply states parity is already enforced during wire parsing; that is plausible but cannot be confirmed from the review context (the decoder is not in scope and the repo is not checked out here), so the concern stands pending an explicit guard or a decoder-invariant citation. F2 (dropping io.ErrUnexpectedEOF acceptance) and F3 (reply budget keyed off peer-supplied encoding) are also still present; F2 now appears intentional given the new corrupt-zlib test, though the interop question it raises is unanswered.
No new commits introduced new defects: the removed io.LimitedReader byte-bound does not re-open a zlib-bomb vector because the 100k count cap short-circuits the decode loop per iteration.
Status of prior findings
- F4 addressed: Fixed in
lnwire/query_short_chan_ids.go:14— themaxDecodedShortChanIDscomment was rewritten to "the maximum number of short channel IDs accepted from a single message. The plain encoding is also bounded by the wire size, so its check is defense in depth," which now covers both encodings and explicitly labels the plain-branch guard as defense-in-depth.
Bot commands
/gateway re-review— re-run after pushing changes/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
|
/gateway dismiss F1 |
|
/gateway dismiss F2 |
|
/gateway dismiss F3 |
|
🚫 Dismissed F1 (major) by @Roasbeef — no reason given Open findings on this PR: 🟠 F2 (major) · 🟡 F3 (minor) |
|
🚫 Dismissed F2 (major) by @Roasbeef — no reason given Open findings on this PR: 🟡 F3 (minor) |
|
🚫 Dismissed F3 (minor) by @Roasbeef — no reason given All findings on this PR are addressed or dismissed. |
2 participants
In this PR, we bound the memory used while decoding and buffering short
channel IDs during channel range synchronization. Compressed replies can
carry more IDs than plain replies, and a query can be answered across many
messages, so the temporary working set can grow well beyond the graph data
we actually need to process.
We now cap decoded ID sets and the aggregate IDs accepted for a single range
query. We also count reply budgets from the encoding carried by each reply,
clear accumulated range state on errors, and reject incomplete compressed
streams. The limits leave headroom above the current graph while keeping the
working set predictable.
See each commit message for a detailed description w.r.t. the incremental
changes.
Testing
go test -count=1 ./lnwire ./discoverygo test -race -count=1 ./lnwire ./discoveryQueryShortChanIDsandReplyChannelRange