swig: guard against silent attribute typos on SWIG objects#3300
Merged
igaw merged 1 commit intolinux-nvme:masterfrom Apr 21, 2026
Merged
swig: guard against silent attribute typos on SWIG objects#3300igaw merged 1 commit intolinux-nvme:masterfrom
igaw merged 1 commit intolinux-nvme:masterfrom
Conversation
igaw
reviewed
Apr 21, 2026
Add __setattr__ to libnvme_global_ctx, libnvme_host, libnvme_subsystem, and libnvme_ctrl. Without this guard, a caller writing e.g. ctrl.dhchap_key = x (a typo for dhchap_ctrl_key) would silently create a new Python instance attribute and discard the value instead of calling the C-level property setter. This has already caused a real bug in nvme-stas. The guard passes through 'this' and '_'-prefixed names (SWIG internals and GC-anchoring refs set by %pythonappend blocks), walks the MRO to invoke writable property setters, and raises AttributeError for everything else — covering both unknown names and %immutable properties. Also reject unknown keys in the ctrl constructor dict. Previously a typo in a dict key (e.g. 'subsysnqn_typo') was silently ignored. Now any unrecognised key raises KeyError at the end of the existing dispatch loop, with no second pass over the dict. Add tests/test-setattr.py to cover: valid writable property, typo, read-only property, unknown attribute, unknown dict key, and missing required dict key. Signed-off-by: Martin Belanger <[email protected]> Assisted-by: Claude Sonnet 4.6 <[email protected]>
81ea89f to
8204c5b
Compare
Collaborator
|
Thanks! |
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.
Problem
SWIG-generated Python classes allow assigning arbitrary attributes on instances. A caller writing:
silently creates a new Python instance attribute instead of calling the C-level property setter. The value is discarded and the bug is invisible at runtime. This has already caused a real bug in
nvme-staswheredhchap_keywas written instead ofdhchap_ctrl_key, meaning the controller DHCHAP key was never actually set. What actually happened is that the propertydhchap_keyhad been renamed todhchap_ctrl_keyin libnvme's python bindings, but had not been changed innvme-stascausing a silent failure.The same silent-failure applies to the ctrl constructor dict: a typo in a key such as
subsysnqn_typowas previously ignored without error.Changes
libnvme/libnvme/nvme.iAdd a
__setattr__guard (via%pythoncode) to four classes:libnvme_global_ctx,libnvme_host,libnvme_subsystem, andlibnvme_ctrl.The guard:
_-prefixed names (GC-anchoring refs set by%pythonappendblocks such asself.__host) viaobject.__setattr__, bypassing property lookup.propertywith anfsetand calls it directly, equivalent to Python's default machinery but without allowing fallback to instance-dict creation.AttributeErrorfor everything else — covering both unknown names and%immutableproperties (which have nofset).Note:
thisownhas anfset, so it is handled correctly by the MRO walk without any special case.In
set_fctx_from_dict(), replace the silent fall-through at the end of the key-dispatchwhileloop with aPyErr_Format(PyExc_KeyError) - return -1. Every known key already hitscontinue; any key that falls through is therefore unknown. This avoids a second pass over the dict.libnvme/libnvme/tests/test-setattr.py(new file)Six test cases:
AttributeErrorimmediately.%immutable(read-only) property raisesAttributeError.AttributeError.KeyError.subsysnqnortransport) raisesKeyError.Testing
meson test -C .build