Optimized and fixed degraded GetBulk responses - #5
Open
syntax1269 wants to merge 2 commits into
Open
Conversation
## v3.1.23 — 2026-09-02 **P1 CORRECTNESS: double-destroy of the parsed request in handlePacket() + double-release guard in ASNPool — root cause of degraded GetBulk responses under trap concurrency and of the host-suite SIGSEGV. Fix verified on ESP-01 hardware and on the full host suite (101/101 green, previously segfaulting).** ### Defects fixed (two, same mechanism) 1. **handlePacket() double-destroyed the parsed request** ([SNMPParser.cpp](src/SNMPParser.cpp)). An explicit `request.~SNMPPacket();` ran after response assembly, then the automatic destructor at scope exit ran again — every pool-backed parse object (requestID/snmpVersion/community `shared_ptr`s + the varbind tree) was destroyed **twice**. Proven by ASan (heap-use-after-free) before this release and now proven live by the new pool alarm on ESP-01 (`DOUBLE RELEASE of slot 21` — slot 21 = first transient slot, exactly where the request's parse objects live). 2. **ASNPool::release() had no double-release guard** ([BERDecode.cpp](src/BERDecode.cpp)). A second release re-ran the destructor and **decremented `usedCount` again**, so the counter drifted below true occupancy. With `usedCount` under-reporting, `rawAlloc()` could hand a still-occupied slot to a second live object: the previous object's data gets overwritten mid-flight (corrupted OIDs/values in responses), and because the counter only ever went *down*, **no exhaustion error was ever logged** — silent corruption. This is the same class of failure as the "agent goes deaf" incidents (HARDWARE_TEST_REPORT.md §2/§9). ### Why GetBulk showed it first GetBulk builds the largest response trees (up to `SNMP_MAX_VARBINDS` varbinds via repeated `findCallback`+clone chains), so it stresses the pool hardest; combined with `SNMPTrap::sendTo()` calling `ASNPool::resetAll()` mid-loop (trap scratch slots stay occupied until the next `snmp.loop()` tick), the double-release counter drift made slot reuse overlap with live bulk-response objects. Measured symptom on ESP-01: `snmpbulkwalk` returning a single varbind per response — with **zero** `EXHAUSTED` logs (counter corruption, not capacity). ### Changes - `SNMPPacket` copy-ctor **shares** the parsed `requestIDPtr/snmpVersionPtr/communityStringPtr` via refcounted `shared_ptr`; `SNMPResponse(request)` copies into the same model. The explicit destructor call was removed; the request tree is now destroyed exactly once at scope exit, after `response.serialiseInto()` — live objects stay alive exactly as long as needed, and the refcount does the ownership bookkeeping. - `ASNPool::release()` now guards double releases: if the slot is already free, the destructor is **not** re-run and `usedCount` is **not** decremented. With `DEBUG>0` a one-shot per-slot alarm logs `ASNPool: DOUBLE RELEASE of slot N detected (caller destroying an object twice)` so residual double-destroy bugs surface instead of corrupting silently. - **Pool telemetry**: `ASNPool::usedCountPeak` (high-water mark since boot) added; the ESP-01 hwtest sketch's 15 s diagnostics now print `pool=<used>/<cap> peak=<peak> perm=<perm>`. - Version 3.1.22 → 3.1.23 (defs.h + library.properties stay in lockstep). ### Hardware verification (ESP-01, esp01_1m, dout, 80 MHz — the non-negotiable flash profile) | Check | Before fix (v3.1.22) | After fix (v3.1.23) | |---|---|---| | `snmpbulkwalk` enterprise tree under trap concurrency | single varbind per response (degraded) | **14/14 varbinds + clean EOM**, before *and* after a trap boundary | | `ASNPool: DOUBLE RELEASE` alarm | n/a (no alarm existed) | fires once on slot 21 during coldStart trap handling — proving the defect, then silence | | Pool occupancy across 100+ requests + 2 traps | corrupted counter (silent) | steady `pool=21/76`, **peak 76/76** saturated and recovered, zero leaks | | Pool exhaustion events | none logged (counter lied) | 0 (counter now truthful) | | Heap trajectory (15 s diag) | ~2,184 B, frag 8% | ~2,104 B flat, frag 8% — no regression | ### Host suite Previously **SIGSEGV** in `Test GetRequestPDU` (same double-destroy under ASan). After the fix: **all tests passed (101 assertions in 10 test cases)**. ### Known remaining limitation (not fixed here) `appendResponseVarBind()` silently drops varbinds past `SNMP_MAX_VARBINDS` (ESP8266_TINY default: 6), so GetBulk responses are capped at 6 varbinds on ESP8266 regardless of `max-repetitions`. Raising `SNMP_MAX_VARBINDS` via sketch `#define` before `#include <SNMP_Agent.h>` lifts the cap (compile-time floor guard already enforces ≥ 4). Truncation is RFC-tolerable for walkers but is silent; a future release should log or report it.
Updated SNMP_POOL_SLOT_SIZE to reflect measured sizes of the largest container types, reducing memory usage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
v3.1.23 — 2026-09-02
P1 CORRECTNESS: double-destroy of the parsed request in handlePacket() + double-release guard in ASNPool — root cause of degraded GetBulk responses under trap concurrency and of the host-suite SIGSEGV. Fix verified on ESP-01 hardware and on the full host suite (101/101 green, previously segfaulting).
Defects fixed (two, same mechanism)
handlePacket() double-destroyed the parsed request (SNMPParser.cpp). An explicit
request.~SNMPPacket();ran after response assembly, then the automatic destructor at scope exit ran again — every pool-backed parse object (requestID/snmpVersion/communityshared_ptrs + the varbind tree) was destroyed twice. Proven by ASan (heap-use-after-free) before this release and now proven live by the new pool alarm on ESP-01 (DOUBLE RELEASE of slot 21— slot 21 = first transient slot, exactly where the request's parse objects live).ASNPool::release() had no double-release guard (BERDecode.cpp). A second release re-ran the destructor and decremented
usedCountagain, so the counter drifted below true occupancy. WithusedCountunder-reporting,rawAlloc()could hand a still-occupied slot to a second live object: the previous object's data gets overwritten mid-flight (corrupted OIDs/values in responses), and because the counter only ever went down, no exhaustion error was ever logged — silent corruption. This is the same class of failure as the "agent goes deaf" incidents (HARDWARE_TEST_REPORT.md §2/§9).Why GetBulk showed it first
GetBulk builds the largest response trees (up to
SNMP_MAX_VARBINDSvarbinds via repeatedfindCallback+clone chains), so it stresses the pool hardest; combined withSNMPTrap::sendTo()callingASNPool::resetAll()mid-loop (trap scratch slots stay occupied until the nextsnmp.loop()tick), the double-release counter drift made slot reuse overlap with live bulk-response objects. Measured symptom on ESP-01:snmpbulkwalkreturning a single varbind per response — with zeroEXHAUSTEDlogs (counter corruption, not capacity).Changes
SNMPPacketcopy-ctor shares the parsedrequestIDPtr/snmpVersionPtr/communityStringPtrvia refcountedshared_ptr;SNMPResponse(request)copies into the same model. The explicit destructor call was removed; the request tree is now destroyed exactly once at scope exit, afterresponse.serialiseInto()— live objects stay alive exactly as long as needed, and the refcount does the ownership bookkeeping.ASNPool::release()now guards double releases: if the slot is already free, the destructor is not re-run andusedCountis not decremented. WithDEBUG>0a one-shot per-slot alarm logsASNPool: DOUBLE RELEASE of slot N detected (caller destroying an object twice)so residual double-destroy bugs surface instead of corrupting silently.ASNPool::usedCountPeak(high-water mark since boot) added; the ESP-01 hwtest sketch's 15 s diagnostics now printpool=<used>/<cap> peak=<peak> perm=<perm>.Hardware verification (ESP-01, esp01_1m, dout, 80 MHz — the non-negotiable flash profile)
snmpbulkwalkenterprise tree under trap concurrencyASNPool: DOUBLE RELEASEalarmpool=21/76, peak 76/76 saturated and recovered, zero leaksHost suite
Previously SIGSEGV in
Test GetRequestPDU(same double-destroy under ASan). After the fix: all tests passed (101 assertions in 10 test cases).Known remaining limitation (not fixed here)
appendResponseVarBind()silently drops varbinds pastSNMP_MAX_VARBINDS(ESP8266_TINY default: 6), so GetBulk responses are capped at 6 varbinds on ESP8266 regardless ofmax-repetitions. RaisingSNMP_MAX_VARBINDSvia sketch#definebefore#include <SNMP_Agent.h>lifts the cap (compile-time floor guard already enforces ≥ 4). Truncation is RFC-tolerable for walkers but is silent; a future release should log or report it.