*: don't use static char buffer in srv6 zapi code

[mjstapp]

Don't use a static buffer for the "locator name" in some zapi apis - the one caller who uses this can supply a buffer.

[greptile-apps]

Greptile Summary

Replaces the static char locator_name[SRV6_LOCNAME_SIZE] buffer inside zapi_srv6_sid_notify_decode with a caller-supplied char *locator_name, size_t loc_size pair, eliminating re-entrancy and thread-safety hazards of the previous static buffer. All three callers are updated: bgpd now provides a proper stack buffer, while isisd and staticd pass NULL, 0 since they don't use the name.

• lib/zclient.c: Bound check (len >= loc_size) guards the null-terminator write; the NULL-locator-name path now calls stream_forward_getp(s, len) to keep the stream correctly positioned (addressing a previously noted stream-alignment gap).
• bgpd/bgp_zebra.c: Stack buffer char loc_name[SRV6_LOCNAME_SIZE] replaces the old dangling char * that pointed into the static region.
• isisd/isis_zebra.c / staticd/static_zebra.c: Unused char *loc_name variables removed; callee now silently consumes the on-wire name bytes.

Confidence Score: 5/5

Safe to merge — a clean removal of a static buffer with correct bounds checks and stream alignment on all code paths.

The change is narrowly scoped: one function signature update and three matching call-site adjustments. The new bounds guard (len >= loc_size) correctly leaves one byte for the null terminator, the stream-advance for the NULL-name path closes the previously flagged misalignment, and no existing behaviour is altered for callers that don't need the locator name.

No files require special attention.

Important Files Changed

Filename	Overview
lib/zclient.c	Core change: removes static buffer, takes caller-supplied locator_name + loc_size; adds proper stream-advance for the NULL-name path and correct null-terminator write after STREAM_GET
lib/zclient.h	Updates the function signature to match the new char *locator_name, size_t loc_size parameters; adds a helpful comment pointing callers to SRV6_LOCNAME_SIZE
bgpd/bgp_zebra.c	Switches char *loc_name to a stack buffer char loc_name[SRV6_LOCNAME_SIZE] and passes it with its size to the updated decode function
isisd/isis_zebra.c	Passes NULL, 0 to the updated decode function since isis doesn't need the locator name; stream is now correctly advanced past the name bytes by the callee
staticd/static_zebra.c	Removes now-unused char *loc_name variable and passes NULL, 0 to the updated decode function, consistent with not needing the locator name

Sequence Diagram

sequenceDiagram
    participant BGP as bgpd/bgp_zebra.c
    participant ISIS as isisd/isis_zebra.c
    participant STATIC as staticd/static_zebra.c
    participant DEC as zapi_srv6_sid_notify_decode()

    BGP->>DEC: loc_name[SRV6_LOCNAME_SIZE], sizeof(loc_name)
    DEC->>DEC: STREAM_GETW(len)
    DEC->>DEC: "if len >= loc_size return false"
    DEC->>DEC: STREAM_GET(locator_name, s, len)
    DEC->>DEC: "locator_name[len] = 0"
    DEC-->>BGP: true, loc_name populated

    ISIS->>DEC: NULL, 0
    DEC->>DEC: STREAM_GETW(len)
    DEC->>DEC: "locator_name==NULL skip size check"
    DEC->>DEC: "else if len>0 stream_forward_getp(s, len)"
    DEC-->>ISIS: true, stream correctly advanced

    STATIC->>DEC: NULL, 0
    DEC->>DEC: STREAM_GETW(len)
    DEC->>DEC: "locator_name==NULL skip size check"
    DEC->>DEC: "else if len>0 stream_forward_getp(s, len)"
    DEC-->>STATIC: true, stream correctly advanced

Loading

_{Reviews (2): Last reviewed commit: "*: don't use static char buffer in srv6 ..." | Re-trigger Greptile}

[greptile-apps]

When locator_name is NULL, STREAM_GETW has already advanced the stream past the 2-byte length field, but the len data bytes that follow are never consumed. The function returns true, leaving the stream offset short by len bytes. This is preserved behaviour from the original code, but if any future caller reads from the stream after this call (or if a protocol extension appends data after the locator name), the stream will be silently misaligned. Adding stream_forward_getp(s, len) in the else if (len > 0) branch would make the success path semantically consistent and future-proof.

Suggested change

	if (locator_name != NULL) {
	/* This is a cstring, and it looks like it's normally sent without
	* its terminal NULL...
	*/
	if (len == 0) {
	locator_name[0] = 0;
	} else {
	STREAM_GET(locator_name, s, len);
	*p_locator_name = locator_name;
	locator_name[len] = 0;
	}
	}
	return true;
	if (locator_name != NULL) {
	/* This is a cstring, and it looks like it's normally sent without
	* its terminal NULL...
	*/
	if (len == 0) {
	locator_name[0] = 0;
	} else {
	STREAM_GET(locator_name, s, len);
	locator_name[len] = 0;
	}
	} else if (len > 0) {
	stream_forward_getp(s, len);
	}
	return true;

Prompt To Fix With AI

This is a comment left during a code review.
Path: lib/zclient.c
Line: 2365-2376

Comment:
When `locator_name` is `NULL`, `STREAM_GETW` has already advanced the stream past the 2-byte length field, but the `len` data bytes that follow are never consumed. The function returns `true`, leaving the stream offset short by `len` bytes. This is preserved behaviour from the original code, but if any future caller reads from the stream after this call (or if a protocol extension appends data after the locator name), the stream will be silently misaligned. Adding `stream_forward_getp(s, len)` in the `else if (len > 0)` branch would make the success path semantically consistent and future-proof.

```suggestion
	if (locator_name != NULL) {
		/* This is a cstring, and it looks like it's normally sent without
		 * its terminal NULL...
		 */
		if (len == 0) {
			locator_name[0] = 0;
		} else {
			STREAM_GET(locator_name, s, len);
			locator_name[len] = 0;
		}
	} else if (len > 0) {
		stream_forward_getp(s, len);
	}
	return true;
```

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

[mjstapp]

yes, that's a valid comment, I will fix it

[mjstapp]

@greptileai review

[donaldsharp]

once ci finishes I will get this in.