vrrrpd: improve error handling in several paths

[mjstapp]

Improve and harden some error-condition handling in several paths, especially packet-processing. Replace several asserts with logs and/or error returns.

[greptile-apps]

Greptile Summary

This PR hardens error handling across the vrrpd packet-processing and socket-setup paths by replacing hard assert() calls with graceful log-and-return patterns, which improves daemon resilience against unexpected runtime conditions.

Key changes:

• vrrp.c: assert(c) in vrrp_socket replaced with zlog_err + goto done when no valid INET address is found; incoming VRRPv3 adver_int values are now masked with 0x0FFF to enforce the 12-bit field boundary defined in RFC 5798.
• vrrp_arp.c: assert(r->family == AF_INET) in vrrp_garp_send_all replaced with zlog_warn + early return; vrrp_build_garp now uses the constant ETH_ALEN instead of ifp->hw_addr_len, which correctly matches the fixed-size GARP_BUFFER_SIZE buffer and prevents a potential overrun on interfaces with non-standard hardware address lengths.
• vrrp_ndisc.c: assert(r->family == AF_INET6) in vrrp_ndisc_una_send replaced with zlog_warn + return -1; the corresponding assert in vrrp_ndisc_una_send_all is removed but no equivalent early-return guard is added at that level, creating a minor inconsistency with the treatment of vrrp_garp_send_all.
• vrrp_packet.c: Two asserts replaced — one in vrrp_pkt_checksum (invalid version → return chksum_pre+1 to guarantee checksum mismatch) and one in vrrp_pkt_parse_datagram (unknown family → VRRP_PKT_VCHECK(false, ...)).

Confidence Score: 4/5

• PR is safe to merge; all changes improve robustness by replacing crash-inducing asserts with graceful log-and-return paths, with one minor consistency gap in vrrp_ndisc.c.
• All assert-to-log replacements are correct and the logic is sound. The ETH_ALEN fix in vrrp_build_garp closes a latent buffer-offset inconsistency. The only notable gap is that vrrp_ndisc_una_send_all lacks the same early-return family guard added to vrrp_garp_send_all, which could produce redundant warning log lines on a wrong-family call — a style/consistency issue, not a correctness bug. No new logic bugs or regressions are introduced.
• vrrpd/vrrp_ndisc.c — missing early-return family check in vrrp_ndisc_una_send_all for consistency with the vrrp_garp_send_all treatment.

Important Files Changed

Filename	Overview
vrrpd/vrrp.c	Replaces an assert(c) with a graceful error log + goto done when no valid INET address is found during socket setup; also adds a 0x0FFF mask to master_adver_interval when receiving VRRPv3 advertisements to properly enforce the 12-bit field boundary.
vrrpd/vrrp_arp.c	Replaces assert(r->family == AF_INET) in vrrp_garp_send_all with a zlog_warn + early return; also corrects vrrp_build_garp to use the constant ETH_ALEN instead of ifp->hw_addr_len, keeping ARP payload offsets consistent with the fixed GARP_BUFFER_SIZE macro and preventing a potential buffer overrun with non-6-byte hw addresses.
vrrpd/vrrp_ndisc.c	Replaces assert(r->family == AF_INET6) in vrrp_ndisc_una_send with a zlog_warn + return -1; removes the assert from vrrp_ndisc_una_send_all but does NOT add an equivalent early-return guard there, creating an inconsistency with the vrrp_garp_send_all treatment and risking N warning logs on a wrong-family call.
vrrpd/vrrp_packet.c	Replaces assert(!"Invalid VRRP protocol version") in vrrp_pkt_checksum with chksum = chksum_pre + 1 (ensuring checksum validation always fails for an invalid version); replaces assert(!"Unknown address family") in vrrp_pkt_parse_datagram with VRRP_PKT_VCHECK(false, ...) for consistent graceful error handling.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Receive VRRP Packet] --> B[vrrp_pkt_parse_datagram]
    B --> C{family == AF_INET\nor AF_INET6?}
    C -- "neither" --> D["VRRP_PKT_VCHECK(false)\nreturn -1\n(was: assert)"]
    C -- "AF_INET/AF_INET6" --> E[Version & Size Checks]
    E --> F[vrrp_pkt_checksum]
    F --> G{Valid version\nin packet?}
    G -- "invalid" --> H["return chksum_pre+1\n(checksum mismatch guaranteed)\n(was: assert)"]
    G -- "valid" --> I[Compute real checksum]
    I --> J[Checksum Match?]
    J -- "no" --> K[return -1]
    J -- "yes" --> L[vrrp_recv_advertisement]
    L --> M{VRRPv3?}
    M -- "yes" --> N["master_adver_interval &= 0x0FFF\n(enforce 12-bit field)"]
    N --> O[vrrp_recalculate_timers]
    M -- "no" --> O

    subgraph Socket Setup
        P[vrrp_socket] --> Q{IPv4 address\nfound on iface?}
        Q -- "no" --> R["zlog_err + goto done\n(was: assert)"]
        Q -- "yes" --> S[setsockopt multicast join]
    end

    subgraph Gratuitous ARP
        T[vrrp_garp_send_all] --> U{family == AF_INET?}
        U -- "no" --> V["zlog_warn + return\n(was: assert)"]
        U -- "yes" --> W[vrrp_garp_send]
    end

    subgraph Neighbor Discovery
        X[vrrp_ndisc_una_send_all] --> Y[vrrp_ndisc_una_send per addr]
        Y --> Z{family == AF_INET6?}
        Z -- "no" --> AA["zlog_warn + return -1\n(was: assert)"]
        Z -- "yes" --> AB[Send ND packet]
    end

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: vrrpd/vrrp_ndisc.c
Line: 193-201

Comment:
**Missing early-return family guard in `_send_all`**

The companion function `vrrp_garp_send_all` in `vrrp_arp.c` received an explicit family check with a single `zlog_warn` and an early return. `vrrp_ndisc_una_send_all` had its `assert` removed but received no equivalent guard. If this function is called with `r->family != AF_INET6` and `r->addrs` contains N addresses, `vrrp_ndisc_una_send` will fire N separate warning log lines instead of one. Adding an early-return check here keeps the two `_send_all` functions consistent:

```suggestion
int vrrp_ndisc_una_send_all(struct vrrp_router *r)
{
	struct listnode *ln;
	struct ipaddr *ip;

	if (r->family != AF_INET6) {
		zlog_warn(VRRP_LOGPFX VRRP_LOGPFX_VRID VRRP_LOGPFX_FAM
			  "Unable to send unsolicited Neighbor Advertisements on %s: not INET6",
			  r->vr->vrid, family2str(r->family),
			  r->mvl_ifp->name);
		return -1;
	}

	for (ALL_LIST_ELEMENTS_RO(r->addrs, ln, ip))
		vrrp_ndisc_una_send(r, ip);

	return 0;
}
```

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

_{Last reviewed commit: "vrrpd: replace some ..."}

[mjstapp]

cleaned up a couple of greptile comments

[mjstapp]

@greptileai review

[Jafaral]

@Mergifyio backport stable/10.6 stable/10.5 stable/10.4 stable/10.3 stable/10.2

[mergify]

backport stable/10.6 stable/10.5 stable/10.4 stable/10.3 stable/10.2

✅ Backports have been created

Details

• #21267 vrrrpd: improve error handling in several paths (backport #21251) has been created for branch stable/10.6
• #21268 vrrrpd: improve error handling in several paths (backport #21251) has been created for branch stable/10.5
• #21269 vrrrpd: improve error handling in several paths (backport #21251) has been created for branch stable/10.4
• #21270 vrrrpd: improve error handling in several paths (backport #21251) has been created for branch stable/10.3
• #21271 vrrrpd: improve error handling in several paths (backport #21251) has been created for branch stable/10.2