lib: replace stream_get_ipv4() with STREAM_GET() in link state parsing

[guoguojia2021]

The stream_get_ipv4() function calls assert(0) when the stream does not have enough data remaining, which causes the entire process to abort. This is problematic for link state parsing functions (ls_parse_node and ls_parse_attributes) because a malformed or truncated message from an external source should not crash the daemon.

Replace all six stream_get_ipv4() call sites with the STREAM_GET() macro, which uses longjmp to jump to the stream_failure label on underflow. Both ls_parse_node() and ls_parse_attributes() already have proper stream_failure cleanup handlers that log an error, free allocated memory, and return NULL, so this change leverages the existing error recovery paths.

The six replaced call sites are:

• ls_parse_node(): node->router_id
• ls_parse_attributes(): attr->standard.local, attr->standard.remote, attr->standard.remote_addr, and two adj_sid neighbor.addr fields (ADJ_PRI_IPV4 and ADJ_BCK_IPV4)

This is consistent with how IPv6 addresses are already read using STREAM_GET() in the same functions, and follows the safe stream reading pattern used throughout the rest of the link state parsing code.

[greptile-apps]

Greptile Summary

This PR replaces six stream_get_ipv4() call sites in ls_parse_node and ls_parse_attributes with the STREAM_GET() macro. The change prevents daemon crashes when a malformed or truncated link-state message is received, routing underflow conditions to the existing stream_failure cleanup labels instead of calling assert(0).

• Correctness of substitution: stream_get_ipv4() uses memcpy to read 4 bytes without byte-swapping, and STREAM_GET(ptr, s, IPV4_MAX_BYTELEN) calls stream_get2() which also does a raw memcpy — the data copy is semantically identical across all six sites.
• Error handling improvement: STREAM_BOUND_WARN (used by stream_get_ipv4()) calls assert(0) on underflow, crashing the process; STREAM_GET instead jumps to stream_failure via longjmp, matching how IPv6 addresses are already handled throughout the same functions.
• Pre-existing cleanup gap: The stream_failure handler in ls_parse_attributes leaks the bitmap allocated by admin_group_init because it never calls admin_group_term — the canonical ls_attributes_del() function performs the full teardown and should be used here instead.

Confidence Score: 4/5

• The six substitutions are mechanically correct and the improved error handling is the right direction. The one concern is the stream_failure cleanup in ls_parse_attributes, which leaks bitmap memory allocated by admin_group_init — using ls_attributes_del() there would close this gap.
• All six stream_get_ipv4() → STREAM_GET() replacements are semantically identical for the data copy and correctly route underflow to the existing cleanup labels. The outstanding issue is a memory leak in the stream_failure handler of ls_parse_attributes: admin_group_term is never called, leaving the bitmap allocated by admin_group_init orphaned on every parse error. This is a real defect in the error path that this PR explicitly relies on.
• lib/link_state.c — specifically the stream_failure handler in ls_parse_attributes (around line 1429)

Important Files Changed

Filename	Overview
lib/link_state.c	Six stream_get_ipv4() call sites replaced with STREAM_GET(); data semantics are identical (both use raw memcpy), error handling improves from assert-abort to stream_failure longjmp. The stream_failure handler in ls_parse_attributes is missing admin_group_term, leaking bitmap memory on parse errors.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Receive link-state message] --> B[ls_parse_node / ls_parse_attributes]
    B --> C{stream underflow?}
    
    subgraph OLD["Before PR (stream_get_ipv4)"]
        C1{underflow?} --> |yes| D1[STREAM_BOUND_WARN\nassert-0 → process abort]
        C1 --> |no| E1[return parsed struct]
    end

    subgraph NEW["After PR (STREAM_GET)"]
        C2{underflow?} --> |yes| D2[goto stream_failure\nlog error + free memory]
        C2 --> |no| E2[return parsed struct]
        D2 --> F2[return NULL]
    end

    C --> C1
    C --> C2

    style D1 fill:#ff6b6b,color:#fff
    style D2 fill:#51cf66,color:#fff
    style F2 fill:#51cf66,color:#fff

Loading

Comments Outside Diff (1)

1. lib/link_state.c, line 1429-1436 (link)

   Missing admin_group_term in stream_failure cleanup

   ls_parse_attributes calls admin_group_init(&attr->ext_admin_group) (line 1280) before any STREAM_GET* calls, which allocates heap memory for the bitmap via bf_init. The stream_failure handler frees attr->srlgs and attr itself, but never calls admin_group_term(&attr->ext_admin_group), leaking that bitmap allocation on every parse error.

   The canonical cleanup function ls_attributes_del() (line 219) already handles this correctly — it calls ls_attributes_srlg_del, then admin_group_term, then XFREE. The stream_failure block should delegate to ls_attributes_del(attr) instead of duplicating the cleanup manually.

   While this defect predates the PR, the explicit goal here is to make the stream_failure path the safe error recovery route for all IPv4 reads, which makes fixing the handler a natural companion to this change.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: lib/link_state.c
   Line: 1429-1436
   
   Comment:
   **Missing `admin_group_term` in stream_failure cleanup**
   
   `ls_parse_attributes` calls `admin_group_init(&attr->ext_admin_group)` (line 1280) before any `STREAM_GET*` calls, which allocates heap memory for the bitmap via `bf_init`. The `stream_failure` handler frees `attr->srlgs` and `attr` itself, but never calls `admin_group_term(&attr->ext_admin_group)`, leaking that bitmap allocation on every parse error.
   
   The canonical cleanup function `ls_attributes_del()` (line 219) already handles this correctly — it calls `ls_attributes_srlg_del`, then `admin_group_term`, then `XFREE`. The `stream_failure` block should delegate to `ls_attributes_del(attr)` instead of duplicating the cleanup manually.
   
   While this defect predates the PR, the explicit goal here is to make the `stream_failure` path the safe error recovery route for all IPv4 reads, which makes fixing the handler a natural companion to this change.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
lib/link_state.c:1429-1436
**Missing `admin_group_term` in stream_failure cleanup**

`ls_parse_attributes` calls `admin_group_init(&attr->ext_admin_group)` (line 1280) before any `STREAM_GET*` calls, which allocates heap memory for the bitmap via `bf_init`. The `stream_failure` handler frees `attr->srlgs` and `attr` itself, but never calls `admin_group_term(&attr->ext_admin_group)`, leaking that bitmap allocation on every parse error.

The canonical cleanup function `ls_attributes_del()` (line 219) already handles this correctly — it calls `ls_attributes_srlg_del`, then `admin_group_term`, then `XFREE`. The `stream_failure` block should delegate to `ls_attributes_del(attr)` instead of duplicating the cleanup manually.

While this defect predates the PR, the explicit goal here is to make the `stream_failure` path the safe error recovery route for all IPv4 reads, which makes fixing the handler a natural companion to this change.

_{Reviews (1): Last reviewed commit: "lib: replace stream_get_ipv4() with STRE..." | Re-trigger Greptile}

[donaldsharp]

Yeah straight up NAK. If we are not properly sending data or not properly checking lengths of data then fix those not this.

[guoguojia2021]

Yeah straight up NAK. If we are not properly sending data or not properly checking lengths of data then fix those not this.

Thanks for the review. I understand your point — fixing the root cause is better than papering over it at the receiver.
However, I'd like to note:

1. Consistency within the same functions: The IPv6 addresses in ls_parse_node() and ls_parse_attributes() already use STREAM_GET() (e.g., line 1225: STREAM_GET(&node->router_id6, s, IPV6_MAX_BYTELEN)). The IPv4 paths using stream_get_ipv4() are the only inconsistency — this PR just aligns them.
2. The stream_failure handler already logs "Could not parse Link State ... Abort!" and returns NULL, which propagates the error to the caller. It doesn't silently succeed.

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request