fix: error storm #3089

Merged

mfreeman451 merged 2 commits from refs/pull/3089/head into staging

2026-03-27 19:55:09 +00:00

mfreeman451 commented

2026-03-27 18:35:44 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #3091
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/3091
Original created: 2026-03-27T18:35:44Z
Original updated: 2026-03-27T19:55:10Z
Original head: carverauto/serviceradar:bug/bmp-collector-broken
Original base: staging
Original merged: 2026-03-27T19:55:09Z by @mfreeman451

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

Imported from GitHub pull request. Original GitHub pull request: #3091 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/3091 Original created: 2026-03-27T18:35:44Z Original updated: 2026-03-27T19:55:10Z Original head: carverauto/serviceradar:bug/bmp-collector-broken Original base: staging Original merged: 2026-03-27T19:55:09Z by @mfreeman451 --- ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test?

Copilot commented

2026-03-27 18:39:53 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR review comment.

Original author: @Copilot
Original URL: https://github.com/carverauto/serviceradar/pull/3091#discussion_r3002606231
Original created: 2026-03-27T18:39:53Z
Original path: rust/bmp-collector/src/bmp.rs
Original line: 302

parse_bgp_open_with_fallback uses the BGP message’s declared length as consume_len on the fallback path. If the OPEN is malformed specifically because the declared length is bogus (too small to include an OPEN header, too large, or outside the valid BGP range), the placeholder parse will still fail and you’ll keep erroring instead of recovering. Consider sanitizing/clamping the declared length (e.g., enforce 19..=4096, and <= remaining bytes; and for OPEN placeholder ensure at least the minimum OPEN length) or falling back to consuming the remaining bytes when the declared length is invalid.

Imported GitHub PR review comment. Original author: @Copilot Original URL: https://github.com/carverauto/serviceradar/pull/3091#discussion_r3002606231 Original created: 2026-03-27T18:39:53Z Original path: rust/bmp-collector/src/bmp.rs Original line: 302 --- `parse_bgp_open_with_fallback` uses the BGP message’s *declared* length as `consume_len` on the fallback path. If the OPEN is malformed specifically because the declared length is bogus (too small to include an OPEN header, too large, or outside the valid BGP range), the placeholder parse will still fail and you’ll keep erroring instead of recovering. Consider sanitizing/clamping the declared length (e.g., enforce 19..=4096, and <= remaining bytes; and for OPEN placeholder ensure at least the minimum OPEN length) or falling back to consuming the remaining bytes when the declared length is invalid.

Copilot commented

2026-03-27 18:39:53 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR review comment.

Original author: @Copilot
Original URL: https://github.com/carverauto/serviceradar/pull/3091#discussion_r3002606251
Original created: 2026-03-27T18:39:53Z
Original path: rust/bmp-collector/src/bmp.rs
Original line: 393

In parse_peer_up_tlvs, encountering an unknown PeerUpTlvType currently breaks out of the loop. BMP Peer Up TLVs can legitimately contain unknown/unsupported types, and breaking means you’ll silently drop any following TLVs even if they’re known. Prefer skipping unknown TLVs by consuming 4 + info_len (when available) and continuing; also you can avoid the extra allocation by passing the byte slice directly to String::from_utf8_lossy instead of creating value_bytes with to_vec().


        // Ensure we have the full TLV (header + value) before processing.
        if data.len() < 4 + info_len {
            break;
        }

        // Unknown TLV types are allowed; skip them but continue parsing the rest.
        let info_type = match PeerUpTlvType::try_from(info_type_raw) {
            Ok(info_type) => info_type,
            Err(_) => {
                let _ = data.split_to(4 + info_len);
                continue;
            }
        };

        let info_value = String::from_utf8_lossy(&data[4..4 + info_len]).to_string();

Imported GitHub PR review comment. Original author: @Copilot Original URL: https://github.com/carverauto/serviceradar/pull/3091#discussion_r3002606251 Original created: 2026-03-27T18:39:53Z Original path: rust/bmp-collector/src/bmp.rs Original line: 393 --- In `parse_peer_up_tlvs`, encountering an unknown `PeerUpTlvType` currently `break`s out of the loop. BMP Peer Up TLVs can legitimately contain unknown/unsupported types, and breaking means you’ll silently drop any following TLVs even if they’re known. Prefer skipping unknown TLVs by consuming `4 + info_len` (when available) and continuing; also you can avoid the extra allocation by passing the byte slice directly to `String::from_utf8_lossy` instead of creating `value_bytes` with `to_vec()`. ```suggestion // Ensure we have the full TLV (header + value) before processing. if data.len() < 4 + info_len { break; } // Unknown TLV types are allowed; skip them but continue parsing the rest. let info_type = match PeerUpTlvType::try_from(info_type_raw) { Ok(info_type) => info_type, Err(_) => { let _ = data.split_to(4 + info_len); continue; } }; let info_value = String::from_utf8_lossy(&data[4..4 + info_len]).to_string(); ```

Copilot commented

2026-03-27 18:39:53 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR review comment.

Original author: @Copilot
Original URL: https://github.com/carverauto/serviceradar/pull/3091#discussion_r3002606279
Original created: 2026-03-27T18:39:53Z
Original path: rust/bmp-collector/src/bmp.rs
Original line: 31

process_bmp_message attaches frame_hex to the error context, and main.rs now logs errors with {err:#}, which will print the full error chain at error level. That means a decode failure will emit up to ~1KB of hex per message and may leak raw routing information into logs. Consider logging only the frame length (and maybe a short prefix/hash) in the error chain, and emitting full hex only under debug! (or behind an opt-in flag).

    debug!(
        "{socket}: received BMP frame: len={} hex={}",
        raw_frame.len(),
        frame_hex_for_log(&raw_frame)
    );
    let message = decode_bmp_message(bytes).with_context(|| {
        format!(
            "{socket}: failed to decode BMP message: frame_len={}",
            raw_frame.len(),

Imported GitHub PR review comment. Original author: @Copilot Original URL: https://github.com/carverauto/serviceradar/pull/3091#discussion_r3002606279 Original created: 2026-03-27T18:39:53Z Original path: rust/bmp-collector/src/bmp.rs Original line: 31 --- `process_bmp_message` attaches `frame_hex` to the error context, and `main.rs` now logs errors with `{err:#}`, which will print the full error chain at error level. That means a decode failure will emit up to ~1KB of hex per message and may leak raw routing information into logs. Consider logging only the frame length (and maybe a short prefix/hash) in the error chain, and emitting full hex only under `debug!` (or behind an opt-in flag). ```suggestion debug!( "{socket}: received BMP frame: len={} hex={}", raw_frame.len(), frame_hex_for_log(&raw_frame) ); let message = decode_bmp_message(bytes).with_context(|| { format!( "{socket}: failed to decode BMP message: frame_len={}", raw_frame.len(), ```