bgpd: fix unaligned pointer cast in bgp_attr_cluster_list()

[guoguojia2021]

When parsing the BGP CLUSTER_LIST attribute, the raw stream buffer pointer is directly cast to (struct in_addr *) via stream_pnt():

cluster_parse((struct in_addr *)stream_pnt(peer->curr), length)

struct in_addr requires 4-byte alignment. The stream buffer pointer, however, depends on the cumulative length of preceding BGP attributes in the UPDATE message. If that cumulative length is not a multiple of 4, stream_pnt() returns an unaligned address.

Fix this by allocating a properly aligned temporary buffer with XMALLOC(), copying the stream data into it with stream_get() (which handles the stream pointer advancement safely), passing the aligned buffer to cluster_parse(), and freeing the temporary buffer.

[greptile-apps]

Greptile Summary

This PR fixes a potential unaligned memory access in bgp_attr_cluster_list() where stream_pnt() was cast directly to struct in_addr * without guaranteeing 4-byte alignment. The fix allocates a properly aligned heap buffer, copies stream bytes into it with stream_get(), passes it to cluster_parse(), and frees it immediately afterward — safe because cluster_hash_alloc always memcpys the data into its own allocation before cluster_parse returns.

• Alignment fix: replaces (struct in_addr *)stream_pnt(...) with an XMALLOC-backed buffer, which malloc guarantees is suitably aligned for any fundamental type.
• Stream pointer advance: stream_get() now advances connection->curr's read pointer, replacing the separate stream_forward_getp() call that the old code relied on.
• Memory safety: cluster_buf is freed after cluster_parse() returns; the returned cluster_list holds its own copy of the data, so there is no use-after-free.

Confidence Score: 4/5

The change is a targeted, narrowly-scoped fix with correct memory ownership — safe to merge with minor style consideration.

The fix correctly allocates aligned memory, copies stream bytes, and frees the buffer only after cluster_parse has made its own copy. The one observation is that stream_get asserts on underflow instead of returning an error, which differs from how the rest of the BGP attribute parser handles stream reads; but the pre-existing length validation makes a live underflow very unlikely.

bgpd/bgp_attr.c — specifically the new stream_get call and whether STREAM_GET / stream_get2 would better fit the surrounding error-handling pattern.

Important Files Changed

Filename	Overview
bgpd/bgp_attr.c	Fixes unaligned pointer cast in bgp_attr_cluster_list() by allocating a heap-aligned buffer, copying stream data into it, and freeing it after cluster_parse() returns; logic is correct but stream_get() asserts on underflow rather than returning an error value.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant BGP as bgp_attr_cluster_list()
    participant XMALLOC as XMALLOC (aligned heap)
    participant SG as stream_get()
    participant CP as cluster_parse()
    participant HG as hash_get()
    participant HA as cluster_hash_alloc()
    participant XF as XFREE

    BGP->>XMALLOC: allocate length bytes (aligned)
    XMALLOC-->>BGP: cluster_buf (4-byte aligned)
    BGP->>SG: stream_get(cluster_buf, curr, length)
    SG-->>BGP: bytes copied + getp advanced
    BGP->>CP: cluster_parse(cluster_buf, length)
    CP->>HG: "hash_get(cluster_hash, &tmp, cluster_hash_alloc)"
    alt cache hit
        HG-->>CP: existing cluster_list (cluster_buf not stored)
    else cache miss
        HG->>HA: "cluster_hash_alloc(&tmp)"
        HA-->>HG: new cluster_list (memcpy of cluster_buf data)
        HG-->>CP: new cluster_list
    end
    CP-->>BGP: "cluster_list*"
    BGP->>XF: XFREE(cluster_buf)
    BGP-->>BGP: return BGP_ATTR_PARSE_PROCEED

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant BGP as bgp_attr_cluster_list()
    participant XMALLOC as XMALLOC (aligned heap)
    participant SG as stream_get()
    participant CP as cluster_parse()
    participant HG as hash_get()
    participant HA as cluster_hash_alloc()
    participant XF as XFREE

    BGP->>XMALLOC: allocate length bytes (aligned)
    XMALLOC-->>BGP: cluster_buf (4-byte aligned)
    BGP->>SG: stream_get(cluster_buf, curr, length)
    SG-->>BGP: bytes copied + getp advanced
    BGP->>CP: cluster_parse(cluster_buf, length)
    CP->>HG: "hash_get(cluster_hash, &tmp, cluster_hash_alloc)"
    alt cache hit
        HG-->>CP: existing cluster_list (cluster_buf not stored)
    else cache miss
        HG->>HA: "cluster_hash_alloc(&tmp)"
        HA-->>HG: new cluster_list (memcpy of cluster_buf data)
        HG-->>CP: new cluster_list
    end
    CP-->>BGP: "cluster_list*"
    BGP->>XF: XFREE(cluster_buf)
    BGP-->>BGP: return BGP_ATTR_PARSE_PROCEED

Loading

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
bgpd/bgp_attr.c:2782
**Consider `stream_get2` / `STREAM_GET` for consistent error handling**

`stream_get()` (void return) calls `assert` internally on underflow, killing the daemon process rather than returning an error that the BGP attribute parser can handle gracefully. The rest of the BGP attribute parsing code uses the `STREAM_GET` macro (backed by `stream_get2`, which returns `bool` and jumps to `stream_failure`) to avoid hard asserts on malformed input. While the upstream length validation at line 2770 makes an underflow unlikely here, switching to `STREAM_GET` with a `stream_failure:` label would be consistent with the surrounding code and guard against any future refactoring that weakens that pre-check.

_{Reviews (1): Last reviewed commit: "bgpd: fix unaligned pointer cast in bgp_..." | Re-trigger Greptile}

[greptile-apps]

Consider stream_get2 / STREAM_GET for consistent error handling

stream_get() (void return) calls assert internally on underflow, killing the daemon process rather than returning an error that the BGP attribute parser can handle gracefully. The rest of the BGP attribute parsing code uses the STREAM_GET macro (backed by stream_get2, which returns bool and jumps to stream_failure) to avoid hard asserts on malformed input. While the upstream length validation at line 2770 makes an underflow unlikely here, switching to STREAM_GET with a stream_failure: label would be consistent with the surrounding code and guard against any future refactoring that weakens that pre-check.

Prompt To Fix With AI

This is a comment left during a code review.
Path: bgpd/bgp_attr.c
Line: 2782

Comment:
**Consider `stream_get2` / `STREAM_GET` for consistent error handling**

`stream_get()` (void return) calls `assert` internally on underflow, killing the daemon process rather than returning an error that the BGP attribute parser can handle gracefully. The rest of the BGP attribute parsing code uses the `STREAM_GET` macro (backed by `stream_get2`, which returns `bool` and jumps to `stream_failure`) to avoid hard asserts on malformed input. While the upstream length validation at line 2770 makes an underflow unlikely here, switching to `STREAM_GET` with a `stream_failure:` label would be consistent with the surrounding code and guard against any future refactoring that weakens that pre-check.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[riw777]

This looks valid.

[riw777]

The one AI comment looks valid, otherwise good.