Skip to content

snmp-ups: prefer exact outlet mappings - #3610

Open
user01010111 wants to merge 2 commits into
networkupstools:masterfrom
user01010111:fix/issue-3360-snmp-setoid
Open

snmp-ups: prefer exact outlet mappings#3610
user01010111 wants to merge 2 commits into
networkupstools:masterfrom
user01010111:fix/issue-3360-snmp-setoid

Conversation

@user01010111

Copy link
Copy Markdown

Summary

Fixes #3360.

snmp-ups supports mapping tables which combine exact outlet.* entries
with numbered outlet templates. su_setOID() previously sent every outlet
name through template parsing before consulting exact table entries. A
registered outlet-shaped name with no matching template could therefore
leave su_find_info() returning NULL and pass that result to
instantiate_info().

The same ordering bypassed exact server-side writable values and commands,
while cleanup inferred allocation ownership from the variable-name prefix.
This changes lookup to prefer exact mappings, returns the existing unknown
status when a numbered template is absent, handles writable server-side
values without an SNMP OID through dstate, and records whether an entry was
instantiated so only dynamically owned copies are freed.

upsd normally forwards only registered writable variables and commands;
arbitrary unpublished outlet-name parser hardening is outside this patch.

Validation

  • A focused hardware-free setvar/instcmd regression links the real snmp-ups
    implementation with the existing mock driver and a minimal outlet mapping.
  • On the unpatched base, the supported exact outlet.desc reproduction exits
    139. GDB stops at the NULL su_info_p->dfl dereference after the template
    lookup fails. The candidate returns STAT_SET_HANDLED and the new value is
    available through dstate.
  • A missing numbered outlet template returns STAT_SET_UNKNOWN without a
    NULL dereference.
  • A valid numbered outlet template and an exact outlet instant command reach
    the SNMP set path and return the expected failure for the deliberately
    invalid test OIDs. An unavailable exact mapping returns the existing
    unknown status, and cleanup completes normally.
  • GCC 16.2.1 and Clang 22.1.8 warning-as-error builds passed for the real
    snmp-ups driver and focused test.
  • The focused test passed under GCC, Clang, AddressSanitizer and
    UndefinedBehaviorSanitizer with no candidate findings.
  • LeakSanitizer could not run under the host's ptrace restrictions, so leak
    detection was disabled for the ASan/UBSan run rather than claimed as
    accepted.
  • The configured driver test suite passed 1/1.
  • The non-ASCII source check, trailing-whitespace and final-newline checks,
    and git diff --check passed.
  • The normal make distcheck-light packaged the candidate and included all
    changed files, then stopped on pre-existing spellcheck findings in unchanged
    NEWS.adoc and docs/man/apcmicrolink.txt.
  • The make SPELLCHECK_ERROR_FATAL=no distcheck-light run built and extracted
    the distribution, and the packaged snmp-ups-setvar-test passed 1/1.
    Overall distcheck remained blocked by unchanged cppunittest because the
    sandbox denied socket(AF_INET, SOCK_STREAM, 0) with EPERM. No candidate
    file was implicated.

No physical SNMP or UPS hardware is required for this regression.

AI assistance

OpenAI Codex gpt-5.6-sol at high reasoning was used for repository
analysis, implementation, review, drafting and validation. The human
contributor remains responsible for reviewing and validating the change.

Look up exact mappings before interpreting outlet names as numbered templates. Return the existing unknown status when a numbered template is absent, update server-side writable values through dstate, and track ownership for instantiated entries.

Add a focused hardware-free regression for exact mappings, missing templates, valid numbered templates, unavailable mappings and cleanup.

AI assistance: OpenAI Codex gpt-5.6-sol at high reasoning. The human contributor remains responsible for reviewing and validating the change.

Signed-off-by: user01010111 <lapses.50.booster@icloud.com>
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

A ZIP file with standard source tarball and another tarball with pre-built docs for commit 48e6877 is temporarily available: NUT-tarballs-PR-3610.zip.

@AppVeyorBot

Copy link
Copy Markdown

Build nut 2.8.5.5182-master completed (commit 31c4222536 by @)

@jimklimov jimklimov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable, but a NEWS.adoc entry would be welcome.

CC @arnaudquette-eaton - would you be able to test this against some SNMP ePDUs?

@jimklimov jimklimov added SNMP Incorrect or missing readings On some devices driver-reported values are systemically off (e.g. x10, x0.1, const+Value, etc.) AI For good or bad, machine tools are upon us. Humans are still the responsible ones. labels Sep 1, 2026
@jimklimov jimklimov added this to the 2.8.6 milestone Sep 1, 2026
@jimklimov jimklimov added bug impacts-release-2.8.5 Issues reported against NUT release 2.8.5 (maybe vanilla or with minor packaging tweaks) labels Sep 1, 2026
@jimklimov

Copy link
Copy Markdown
Member

The mentioned "exits 139" I suppose means a SIGSEGV (128 + 11) which is usually linked to a NULL pointer dereference.

@biergaizi

Copy link
Copy Markdown
Contributor

This PR commits drivers/snmp-ups-setvar-test.c into the tree, is that expected?

NUT doesn't have a central location for unit tests, and most drivers are not designed for unit tests, the build system doesn't do unit tests for individual regressions, so an ad-hoc unit test drivers/snmp-ups-setvar-test.c for temporarily testing a single bug doesn't fit to the existing code organization.

@jimklimov

Copy link
Copy Markdown
Member

Thanks @biergaizi - I somehow missed the location. There is precedent for piecemeal parser checks under tests, so the file and Makefile change could make better sense in that directory.

Move the focused SNMP setvar regression from drivers/ to tests/ and wire it through the existing linked-driver-source pattern.

Restore drivers/Makefile.am to the normal driver-only build and add the requested NEWS entry for issue networkupstools#3360.

AI assistance: OpenAI Codex gpt-5.6-sol at high reasoning. The human contributor remains responsible for reviewing and validating the change.

Signed-off-by: user01010111 <lapses.50.booster@icloud.com>
@user01010111

Copy link
Copy Markdown
Author

Thanks @biergaizi and @jimklimov. This was an agent oversight: it concentrated on proving the runtime behaviour and regression, but did not compare the nearest repository-placement and release-note precedents before preparing the original change. The test should have been placed under tests from the outset.

The branch has now been updated to:

The regression remains focused on the affected su_setOID() paths: exact server-side outlet variables and commands, numbered outlet templates, missing templates, and unavailable mappings.

Validation after the relocation:

  • focused Automake regression: 1/1 passed with GCC;
  • direct regression builds passed with GCC and strict Clang;
  • GCC ASan/UBSan run passed;
  • the real snmp-ups target built with GCC and Clang;
  • distribution generation includes the regression under tests/, with no copy under drivers/; and
  • whitespace, non-ASCII and final-newline checks passed.

The regression source itself is byte-for-byte unchanged. Thank you for catching the organisational mismatch.

@AppVeyorBot

Copy link
Copy Markdown

@AppVeyorBot

Copy link
Copy Markdown

Build nut 2.8.5.5190-master failed (commit 0c578d38c8 by @)

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

Labels

AI For good or bad, machine tools are upon us. Humans are still the responsible ones. bug impacts-release-2.8.5 Issues reported against NUT release 2.8.5 (maybe vanilla or with minor packaging tweaks) Incorrect or missing readings On some devices driver-reported values are systemically off (e.g. x10, x0.1, const+Value, etc.) SNMP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

snmp-ups: not checking for possibly NULL returns before dereference and use

4 participants