php / php/php-src

ext/snmp: minor cleanup in suffix-as-keys walk path

Open
#21,364 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Extension: snmp
Dominant language
C
Stars
40.4k
Forks
8.1k
Avg merge
2d 13h
Merged PRs (30d)
96

Description

Three pre-existing issues in the SNMP_USE_SUFFIX_AS_KEYS | SNMP_CMD_WALK code path in php_snmp() (ext/snmp/snmp.c), noticed while reviewing #21341.

1. Redundant snprint_objid call

snprint_objid(buf2, sizeof(buf2), vars->name, vars->name_length); // writes full OID
if (rootlen <= vars->name_length && snmp_oid_compare(root, rootlen, vars->name, rootlen) == 0) {
    // snprintf loop overwrites buf2 entirely with the suffix

When the if condition is true (the normal walk case), buf2 is written twice and the snprint_objid result is discarded. The call only has effect as a fallback when the condition fails — but in that case SUFFIX_AS_KEYS mode silently returns a full OID as the key, mixing formats in the result array.

Suggestion: move the snprint_objid call into the else branch so it only runs when needed.

2. Empty-suffix key collision

When rootlen == vars->name_length (exact match on the walk root OID), the while loop never executes, pos stays 0, and buf2[0] = '\0'. The entry is added with key "". Multiple such OIDs would clobber each other in the returned array.

Worth deciding whether this case should be excluded, use the full OID as fallback, or is simply impossible in practice (in which case an assertion would document the invariant).

3. int count signed/unsigned comparison

count is declared int at line 111 but compared against vars->name_length (size_t) throughout the function, producing -Wsign-compare warnings. Changing to size_t is mechanical but touches multiple loop sites across the function.

Happy to send a PR covering all three if the approach looks reasonable.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in ext/snmp/snmp.c at php_snmp() and trace the SNMP_USE_SUFFIX_AS_KEYS | SNMP_CMD_WALK path. Review the suffix construction, the exact-root case, and every comparison involving count and vars->name_length. Done means the redundant formatting is removed, the empty-key behavior is decided and covered, and the signed/unsigned warnings are addressed.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
networking
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.