RDKB-63540: [XB10] RIPv2 enable and mode restriction for single build

[nvenka781]

RDKB-63540: [XB10] RIPv2 enable and mode restriction for single build

Reason for change:
Adding feature support check for RIPv2
Added stub check for conflicting features like MAP-T

Test Procedure: Build

Risks: Low

Signed-off-by: Nagalakshmi Venkataraman nvenka781@cable.comcast.com

[Copilot]

Pull request overview

Adds OneStack feature gating for RIPv2 on XB10 builds so RIP can be rejected when the current mode doesn’t support it, and introduces a stub for blocking RIP when conflicting features (e.g., MAP-T) are enabled.

Changes:

• Include and link rdkb_feature_mode_gate for OneStack builds.
• Extend existing RIP logging/flow guards to include _XB10_PRODUCT_REQ_.
• Add runtime gating in rip_start() to reject RIP when FEATURE_RIPv2 isn’t supported (and a placeholder conflict check).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
source/service_routed/service_routed.c	Adds OneStack RIPv2 support/mode gating and extends product guards to XB10.
source/service_routed/Makefile.am	Links -lrdkb_feature_mode_gate when ONESTACK_PRODUCT_REQ is enabled and adjusts -lnet LDFLAGS handling.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

New RIPv2 feature/mode gating logic in rip_start() is currently untested by the existing service_routed gtests (which exercise rip_start(), but won't hit this branch unless the relevant product/onestack macros are enabled and isFeatureSupportedInCurrentMode() is mocked). Adding a unit test for the "unsupported mode" path would help prevent regressions (e.g., verifying rip-status updates and that routed startup behavior matches expectations).

Suggested change

	t2_event_d("RIP_NotSupported", 1);
	return -1;
	}
	else if (IsRIPConflictingFeaturesEnabled())
	{
	DEG_PRINT("RIP enable rejected due to conflicting features\n");
	t2_event_d("RIP_NotSupported", 1);
	t2_event_d("RIP_NotSupported", 1);
	sysevent_set(sr->sefd, sr->setok, "rip-status", "error", 0);
	return -1;
	}
	else if (IsRIPConflictingFeaturesEnabled())
	{
	DEG_PRINT("RIP enable rejected due to conflicting features\n");
	t2_event_d("RIP_NotSupported", 1);
	sysevent_set(sr->sefd, sr->setok, "rip-status", "error", 0);

[Copilot]

Adding XB10_PRODUCT_REQ to this block enables IsFileExists() (which uses struct stat/stat()), but <sys/stat.h> is only included for _CBR/_BWG/_CBR2. XB10 builds may fail to compile with unknown 'struct stat' / implicit 'stat'. Include <sys/stat.h> when XB10_PRODUCT_REQ is defined (or include it unconditionally in the IsFileExists block).

[Copilot]

On unsupported mode / conflicting features, rip_start() returns -1 after setting "rip-status" to "starting". This leaves rip-status stuck at "starting" and also causes serv_routed_start() to fail ("routed-status" becomes "error"), even though the intended outcome seems to be "don't start RIP" rather than "fail routed". Consider moving the feature/conflict gate before setting statuses/generating config, and either set an appropriate terminal rip-status (e.g., "error"/"not-supported") and return 0, or explicitly handle this case so routed can still start RA/routes/firewall.

[Copilot]

IsRIPConflictingFeaturesEnabled() introduces the BOOL return type, but this file otherwise uses C99 bool (and BOOL is not declared anywhere in this translation unit). This can break builds depending on include order/toolchain. Prefer using bool (from <stdbool.h>) and return true/false, or include the header that defines BOOL within the same compile-time guard.

Suggested change

	static BOOL IsRIPConflictingFeaturesEnabled(void)
	{
	// TODO: Add check to see if any conflicting feature of RIP
	// like MAP-T are enabled
	return FALSE;
	static bool IsRIPConflictingFeaturesEnabled(void)
	{
	// TODO: Add check to see if any conflicting feature of RIP
	// like MAP-T are enabled
	return false;

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

When RIP is rejected due to unsupported mode, the function returns -1 after setting "rip-status" to "starting", leaving the RIP state stuck and also causing serv_routed_start() to fail (it treats rip_start() != 0 as fatal and sets routed-status=error). Before returning, set an appropriate "rip-status" (e.g., "error"/"stopped" or a dedicated "not-supported"), and consider returning 0 so routed can still come up while RIP remains disabled in this mode.

Suggested change

	return -1;
	}
	else if (IsRIPConflictingFeaturesEnabled())
	{
	DEG_PRINT("RIP enable rejected due to conflicting features\n");
	t2_event_d("RIP_NotSupported", 1);
	return -1;
	sysevent_set(sr->sefd, sr->setok, "rip-status", "not-supported", 0);
	return 0;
	}
	else if (IsRIPConflictingFeaturesEnabled())
	{
	DEG_PRINT("RIP enable rejected due to conflicting features\n");
	t2_event_d("RIP_NotSupported", 1);
	sysevent_set(sr->sefd, sr->setok, "rip-status", "not-supported", 0);
	return 0;

[Copilot]

When RIP is rejected due to conflicting features (MAP-T), this returns -1 without updating "rip-status" from "starting", and it will also cause serv_routed_start() to fail (routed-status=error). Update "rip-status" before returning, and consider treating this as a non-fatal condition for routed (return 0) while keeping RIP disabled.

Suggested change

	return -1;
	}
	else if (IsRIPConflictingFeaturesEnabled())
	{
	DEG_PRINT("RIP enable rejected due to conflicting features\n");
	t2_event_d("RIP_NotSupported", 1);
	return -1;
	sysevent_set(sr->sefd, sr->setok, "rip-status", "disabled", 0);
	return 0;
	}
	else if (IsRIPConflictingFeaturesEnabled())
	{
	DEG_PRINT("RIP enable rejected due to conflicting features\n");
	t2_event_d("RIP_NotSupported", 1);
	sysevent_set(sr->sefd, sr->setok, "rip-status", "disabled", 0);
	return 0;

[github-actions]

Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

---

I have read the CLA Document and I hereby sign the CLA

---

7 out of 8 committers have signed the CLA.
✅ (hirrangandhid)[https://github.com/hirrangandhid]
✅ (Amaresh-Kotekal)[https://github.com/Amaresh-Kotekal]
✅ (Suganya-Sugumar)[https://github.com/Suganya-Sugumar]
✅ (bunnam988)[https://github.com/bunnam988]
✅ (GoutamD2905)[https://github.com/GoutamD2905]
✅ (Krithiksha11)[https://github.com/Krithiksha11]
✅ (aadhithan01)[https://github.com/aadhithan01]
❌ @Khan3033
_{You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.}

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The iptables rule emitted for MAP-E uses -p icmp ... MASQUERADE --to-ports ... which is not valid (the --to-ports option only applies to TCP/UDP/etc.). This will cause iptables-restore to fail and can prevent firewall/NAT from coming up. Drop --to-ports for ICMP (or remove that rule) and consider using the MAPE_TUNNEL_INTERFACE constant instead of hardcoding "ip6tnl".

Suggested change

	fprintf(fp, "-A postrouting_towan -o %s -p udp -j MASQUERADE --to-ports %hu-%hu\n", "ip6tnl", min_port, max_port);
	fprintf(fp,"-A postrouting_towan -o %s -p tcp -j MASQUERADE --to-ports %hu-%hu\n", "ip6tnl", min_port, max_port);
	fprintf(fp, "-A postrouting_towan -o %s -p icmp -j MASQUERADE --to-ports %hu-%hu\n", "ip6tnl", min_port, max_port);
	fprintf(fp, "-A postrouting_towan -o %s -j MASQUERADE\n", "ip6tnl");
	fprintf(fp, "-A postrouting_towan -o %s -p udp -j MASQUERADE --to-ports %hu-%hu\n", MAPE_TUNNEL_INTERFACE, min_port, max_port);
	fprintf(fp,"-A postrouting_towan -o %s -p tcp -j MASQUERADE --to-ports %hu-%hu\n", MAPE_TUNNEL_INTERFACE, min_port, max_port);
	fprintf(fp, "-A postrouting_towan -o %s -p icmp -j MASQUERADE\n", MAPE_TUNNEL_INTERFACE);
	fprintf(fp, "-A postrouting_towan -o %s -j MASQUERADE\n", MAPE_TUNNEL_INTERFACE);

[Copilot]

prepare_mape_rules() writes the PREROUTING ACCEPT rule using current_wan_ifname even when MAP-E is enabled. Since the rest of the MAP-E logic switches rules to MAPE_TUNNEL_INTERFACE when isMAPEReady is true, this rule likely won’t match the actual ingress interface in MAP-E mode. Use isMAPEReady ? MAPE_TUNNEL_INTERFACE : current_wan_ifname (or otherwise derive the correct ingress ifname) here as well.

Suggested change

	fprintf(mangle_fp, "-A PREROUTING -p all -i %s -d %s -j ACCEPT\n", current_wan_ifname, lan_prefix);
	const char *mape_ingress_ifname = isMAPEReady ? MAPE_TUNNEL_INTERFACE : current_wan_ifname;
	fprintf(mangle_fp, "-A PREROUTING -p all -i %s -d %s -j ACCEPT\n", mape_ingress_ifname, lan_prefix);

[Copilot]

In MAP-E mode, the ACCEPT rule in the AF_INET branch uses isMAPEReady ? MAPE_TUNNEL_INTERFACE : interface, but the webui_limit rule above still matches on interface. That inconsistency can cause remote-management traffic (on the webui-limited ports) to miss the intended rule when MAP-E traffic arrives on the tunnel interface. Use the same interface selection for both rules (either always the passed-in interface, or always the MAP-E tunnel when isMAPEReady).

[Copilot]

The PR description focuses on RIPv2 enable/mode restriction for XB10/Single build, but this change set also includes substantial unrelated updates (MAP-E firewall/NAT logic, self-heal cron job scheduling, voice syscfg mapping, pmon/dhcp_server monitoring changes, defaults files, and changelog bump). Please either scope this PR down or update the description (and risk/testing section) to cover these additional behavioral changes.

[Copilot]

For ONESTACK_PRODUCT_REQ, defaultsFile = onestackutils_get_defaults_file() is used directly in logging and passed to fopen(). If that function returns NULL (or an empty string), this will lead to undefined behavior / crash. Add a NULL/empty check and fall back to DEFAULT_FILE (or return an error) before using defaultsFile.

Suggested change

	defaultsFile = onestackutils_get_defaults_file();
	APPLY_PRINT("%s - onestackutils_get_defaults_file returned %s\n", __FUNCTION__, defaultsFile);
	const char *onestackDefaultsFile = onestackutils_get_defaults_file();
	if (onestackDefaultsFile == NULL || onestackDefaultsFile[0] == '\0')
	{
	APPLY_PRINT("%s - onestackutils_get_defaults_file returned invalid path, falling back to DEFAULT_FILE %s\n",
	__FUNCTION__, DEFAULT_FILE);
	defaultsFile = DEFAULT_FILE;
	}
	else
	{
	defaultsFile = onestackDefaultsFile;
	APPLY_PRINT("%s - onestackutils_get_defaults_file returned %s\n", __FUNCTION__, defaultsFile);
	}

[Copilot]

The values from syscfg (AggressiveInterval, resource_monitor_interval, ConnTest_PingInterval) are written directly into the crontab line via echo "*/$AGGRESSIVE_INTERVAL * * * * ..." without any sanitization. If an attacker can influence these syscfg keys (e.g., via config or management interfaces), they could inject newlines or non-numeric content to create arbitrary cron entries and achieve command execution as root. Ensure these intervals are strictly validated as numeric (e.g., only digits within an expected range) before using them in the cron expression, or otherwise sanitize/escape them before writing to $CRONTAB_FILE.