Fix DestroyObjectsChecked aborting batches that name an absent id#461
Open
yosuke-wolfssl wants to merge 1 commit into
Open
Fix DestroyObjectsChecked aborting batches that name an absent id#461yosuke-wolfssl wants to merge 1 commit into
yosuke-wolfssl wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Aligns wh_Nvm_DestroyObjectsChecked and wh_Server_KeystoreEraseKeyChecked with the “absent IDs/keys are not an error” contract, while also preventing unnecessary flash partition rewrites when a non-empty destroy list contains no present objects.
Changes:
- Update
wh_Nvm_DestroyObjectsCheckedto tolerateWH_ERROR_NOTFOUNDfrom policy checks so a batch destroy won’t abort just because some IDs are absent. - Fix
wh_Server_KeystoreEraseKeyCheckedto stop masking cache-eviction policy denials when NVM is attached, while treating missing keys as a successful erase. - Avoid no-op flash wear by skipping replication in the flash backend when a non-empty destroy list matches no present objects; add/adjust tests and docs for the unified semantics.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| wolfhsm/wh_server_keystore.h | Documents checked erase semantics: missing key is OK; policy denial returns WH_ERROR_ACCESS. |
| wolfhsm/wh_nvm.h | Documents checked destroy semantics: absent IDs do not error; batch still enforces policy on present objects. |
| src/wh_server_keystore.c | Propagates ACCESS from checked eviction; treats missing key as OK; uses checked NVM destroy when attached. |
| src/wh_nvm.c | Allows WH_ERROR_NOTFOUND during per-ID policy checks in wh_Nvm_DestroyObjectsChecked. |
| src/wh_nvm_flash.c | Skips partition replication when a non-empty destroy list matches nothing (prevents unnecessary flash wear). |
| test/wh_test_nvmflags.c | Adds client-side coverage for mixed present/absent batch destroy and “policy violation aborts batch” behavior. |
| test-refactor/wh_test_list.c | Registers new server-side refactor test whTest_NvmPolicyChecked. |
| test-refactor/server/wh_test_nvm_policy.c | Adds server-side tests for batch tolerance, no-churn all-absent destroy, missing-key erase OK, and ACCESS propagation regression guard. |
| test-refactor/server/wh_test_nvm_optional.c | Updates expectations: checked erase returns OK for missing key even without NVM. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #461
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
No new issues found in the changed files. ✅
Frauschi
approved these changes
Jul 23, 2026
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.
Fix DestroyObjectsChecked aborting batches that name an absent id
Fixes f_6645.
Problem
wh_Nvm_DestroyObjectsCheckedreturnedWH_ERROR_NOTFOUNDif any id in abatch was absent, destroying none of the objects — contradicting the
DestroyObjectscontract ("IDs not present do not cause an error") and thesibling
wh_Nvm_AddObjectChecked, which already toleratesNOTFOUND. Reachablefrom clients via
wh_Client_NvmDestroyObjectsandwh_Client_KeyErase.Fix
wh_Nvm_DestroyObjectsCheckedtoleratesNOTFOUND— an absent id isskipped; only
ACCESS/BADARGSabort the batch.wh_Server_KeystoreEraseKeyCheckedstops masking policy denials — itpreviously discarded the evict return code when NVM was attached, so a revoked
cache-only key would report a successful erase while staying usable. It now
propagates
ACCESSand tolerates onlyNOTFOUND.longer replicates the whole partition. The skip lives in
wh_NvmFlash_DestroyObjects(not the backend-agnostic
wh_nvm.c), keeping the checked and unchecked APIsidentical on every backend; zero-count compaction still replicates.
Behavior changes (client-visible)
wh_Client_KeyErase(missing key) andwh_Client_NvmDestroyObjects(any listedid absent) now return
WH_ERROR_OKinstead ofWH_ERROR_NOTFOUND, matching theDestroyObjectscontract. Add a## Bug FixesChangeLog line when the releasesection is cut.
Tests
test/wh_test_nvmflags.c— mixed batch destroys the present ids and returns OK;a policy violation still aborts the whole batch.
test-refactor/server/wh_test_nvm_policy.c(new) — batch tolerance, all-absentno-churn (via reclaim accounting) + zero-count compaction, missing-key erase,
and the cached-only revoked-key
ACCESSregression guard.wh_nvm.h/wh_server_keystore.h;wh_test_nvm_optional.cassertion updated to the unified semantics.
Verification
refactored suite 38 passed / 0 failed; legacy suite clean on default,
DMA=1,NOCRYPTO=1,THREADSAFE=1under-std=c90 -Werror. Negative controls confirmeach guard is load-bearing (reverting the NVM tolerance reproduces the 6645
symptom exactly).