Skip to content

net-snmp: fix literal string test in snmpd_sink_add#28013

Merged
stintel merged 1 commit intoopenwrt:masterfrom
librick:fix-snmpd-sink-literal
Dec 14, 2025
Merged

net-snmp: fix literal string test in snmpd_sink_add#28013
stintel merged 1 commit intoopenwrt:masterfrom
librick:fix-snmpd-sink-literal

Conversation

@librick
Copy link

@librick librick commented Dec 4, 2025

The function snmpd_sink_add() has a guard clause that tests the literal string "section", not the variable value "$section".

The test [ -n "section" ] always evaluates to true because the string literal "section" is non-empty, making the check useless.

This function is only called internally with hardcoded arguments, so the bug has no actual impact currently. For the same reason, this change should not break existing configurations. However, I think it should be fixed so future callers do not have a false sense of security.

📦 Package Details

Maintainer: @stintel
(You can find this by checking the history of the package Makefile.)

Description:

This modifies the snmpd_sink_add function, used to create SNMP configuration directives including trapsink, trap2sink, and informsink, to fix a semantically useless non-empty string literal test.


🧪 Run Testing Details

  • OpenWrt Version: 21.02-SNAPSHOT
  • OpenWrt Target/Subtarget: Redacted for privacy reasons (bug isn't platform-specific)
  • OpenWrt Device: Redacted for privacy reasons (bug isn't platform-specific)

✅ Formalities

  • I have reviewed the CONTRIBUTING.md file for detailed contributing guidelines.

@stintel
Copy link
Member

stintel commented Dec 14, 2025

@GeorgeSapkin any idea why CI shits the bed?

The function snmpd_sink_add() has a guard clause that tests the literal
string "section", not the variable value "$section".

The test `[ -n "section" ]` always evaluates to true because the string
literal "section" is non-empty, making the check useless.

This function is only called internally with hardcoded arguments, so the
bug has no actual impact currently. For the same reason, this change
should not break existing configurations. However, I think it should be
fixed so future callers do not have a false sense of security.

Signed-off-by: Eric McDonald <librick-openwrt@proton.me>
@GeorgeSapkin GeorgeSapkin force-pushed the fix-snmpd-sink-literal branch from 743e639 to bb4018e Compare December 14, 2025 21:10
@stintel
Copy link
Member

stintel commented Dec 14, 2025

@GeorgeSapkin thanks for having a look, but could you explain what you did? Was this already fixed and this PR just had to rebased on master?

@GeorgeSapkin
Copy link
Member

There was an issue with posting formality check status comments to PRs due to the default token permissions. I reverted this in master a few days ago while they are being figured out. I saw your PR was from some time before that, so I rebased.

@stintel stintel merged commit 93983e5 into openwrt:master Dec 14, 2025
12 checks passed
@stintel
Copy link
Member

stintel commented Dec 14, 2025

@librick thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants