Skip to content

Commit 8204c5b

Browse files
author
Martin Belanger
committed
swig: guard against silent attribute typos on SWIG objects
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]>
1 parent 6f3cfb1 commit 8204c5b

3 files changed

Lines changed: 130 additions & 1 deletion

File tree

libnvme/libnvme/meson.build

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ if want_python
8484
[ 'sigsegv-during-gc', [ files('tests/gc.py'), ] ],
8585
[ 'read-nbft-file', [ files('tests/test-nbft.py'), '--filename', join_paths(meson.current_source_dir(), 'tests', 'NBFT') ] ],
8686
[ 'object-properties-and-errors', [ files('tests/test-objects.py'), ] ],
87+
[ 'setattr-guards', [ files('tests/test-setattr.py'), ] ],
8788
]
8889
foreach test: py_tests
8990
description = test[0]

libnvme/libnvme/nvme.i

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,14 @@ static inline PyObject *Py_NewRef(PyObject *obj)
105105
cfg = libnvmf_context_get_fabrics_config(fctx);
106106

107107
while (PyDict_Next(dict, &pos, &key, &value)) {
108+
/* Already consumed above via dict_get_str() */
109+
if (!PyUnicode_CompareWithASCIIString(key, "subsysnqn") ||
110+
!PyUnicode_CompareWithASCIIString(key, "transport") ||
111+
!PyUnicode_CompareWithASCIIString(key, "traddr") ||
112+
!PyUnicode_CompareWithASCIIString(key, "trsvcid") ||
113+
!PyUnicode_CompareWithASCIIString(key, "host_traddr") ||
114+
!PyUnicode_CompareWithASCIIString(key, "host_iface"))
115+
continue;
108116
if (!PyUnicode_CompareWithASCIIString(key, "queue_size")) {
109117
libnvme_fabrics_config_set_queue_size(cfg, PyLong_AsLong(value));
110118
continue;
@@ -216,7 +224,8 @@ static inline PyObject *Py_NewRef(PyObject *obj)
216224
has_persistent = true;
217225
continue;
218226
}
219-
/* Unknown keys are silently ignored */
227+
PyErr_Format(PyExc_KeyError, "unknown ctrl config key: '%U'", key);
228+
return -1;
220229
}
221230

222231
if (hostnqn || hostid)
@@ -686,6 +695,20 @@ struct libnvme_ns {
686695
while h:
687696
yield h
688697
h = libnvme_next_host(self, h)
698+
def __setattr__(self, name, value):
699+
if name == 'this' or name.startswith('_'):
700+
object.__setattr__(self, name, value)
701+
return
702+
for klass in type(self).__mro__:
703+
attr = klass.__dict__.get(name)
704+
if attr is not None:
705+
if isinstance(attr, property) and attr.fset is not None:
706+
attr.fset(self, value)
707+
return
708+
break
709+
raise AttributeError(
710+
f"'{type(self).__name__}' object has no writable attribute '{name}'"
711+
)
689712
%}
690713
void refresh_topology() {
691714
libnvme_refresh_topology($self);
@@ -750,6 +773,20 @@ struct libnvme_ns {
750773
while s:
751774
yield s
752775
s = libnvme_next_subsystem(self, s)
776+
def __setattr__(self, name, value):
777+
if name == 'this' or name.startswith('_'):
778+
object.__setattr__(self, name, value)
779+
return
780+
for klass in type(self).__mro__:
781+
attr = klass.__dict__.get(name)
782+
if attr is not None:
783+
if isinstance(attr, property) and attr.fset is not None:
784+
attr.fset(self, value)
785+
return
786+
break
787+
raise AttributeError(
788+
f"'{type(self).__name__}' object has no writable attribute '{name}'"
789+
)
753790
%}
754791
}
755792

@@ -806,6 +843,20 @@ struct libnvme_ns {
806843
while ns:
807844
yield ns
808845
ns = libnvme_subsystem_next_ns(self, ns)
846+
def __setattr__(self, name, value):
847+
if name == 'this' or name.startswith('_'):
848+
object.__setattr__(self, name, value)
849+
return
850+
for klass in type(self).__mro__:
851+
attr = klass.__dict__.get(name)
852+
if attr is not None:
853+
if isinstance(attr, property) and attr.fset is not None:
854+
attr.fset(self, value)
855+
return
856+
break
857+
raise AttributeError(
858+
f"'{type(self).__name__}' object has no writable attribute '{name}'"
859+
)
809860
%}
810861
%immutable name;
811862
const char *name;
@@ -1035,6 +1086,20 @@ struct libnvme_ns {
10351086
while ns:
10361087
yield ns
10371088
ns = libnvme_ctrl_next_ns(self, ns)
1089+
def __setattr__(self, name, value):
1090+
if name == 'this' or name.startswith('_'):
1091+
object.__setattr__(self, name, value)
1092+
return
1093+
for klass in type(self).__mro__:
1094+
attr = klass.__dict__.get(name)
1095+
if attr is not None:
1096+
if isinstance(attr, property) and attr.fset is not None:
1097+
attr.fset(self, value)
1098+
return
1099+
break
1100+
raise AttributeError(
1101+
f"'{type(self).__name__}' object has no writable attribute '{name}'"
1102+
)
10381103
%}
10391104
}
10401105

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
# SPDX-License-Identifier: LGPL-2.1-or-later
2+
# This file is part of libnvme.
3+
# Copyright (c) 2026, Dell Technologies Inc. or its subsidiaries.
4+
# Authors: Martin Belanger <[email protected]>
5+
"""Tests that __setattr__ guards on SWIG-generated classes raise on bad names."""
6+
7+
import unittest
8+
from libnvme import nvme
9+
10+
11+
class TestCtrlSetattr(unittest.TestCase):
12+
13+
def setUp(self):
14+
self.ctx = nvme.global_ctx()
15+
self.ctrl = nvme.ctrl(self.ctx, {
16+
'subsysnqn': nvme.NVME_DISC_SUBSYS_NAME,
17+
'transport': 'loop',
18+
})
19+
20+
def test_valid_writable_property(self):
21+
"""Writing a valid writable property must not raise."""
22+
self.ctrl.discovery_ctrl = True
23+
24+
def test_typo_raises(self):
25+
"""A typo in a property name must raise AttributeError immediately."""
26+
with self.assertRaises(AttributeError):
27+
self.ctrl.dhchap_key = 'somekey' # correct name is dhchap_ctrl_key
28+
29+
def test_readonly_raises(self):
30+
"""Writing a read-only (%immutable) property must raise AttributeError."""
31+
with self.assertRaises(AttributeError):
32+
self.ctrl.transport = 'tcp'
33+
34+
def test_unknown_attr_raises(self):
35+
"""A completely unknown attribute name must raise AttributeError."""
36+
with self.assertRaises(AttributeError):
37+
self.ctrl.does_not_exist = 42
38+
39+
40+
class TestCtrlDictValidation(unittest.TestCase):
41+
42+
def setUp(self):
43+
self.ctx = nvme.global_ctx()
44+
45+
def test_unknown_dict_key_raises(self):
46+
"""An unknown key in the constructor dict must raise KeyError."""
47+
with self.assertRaises(KeyError):
48+
nvme.ctrl(self.ctx, {
49+
'subsysnqn': nvme.NVME_DISC_SUBSYS_NAME,
50+
'transport': 'loop',
51+
'typo_key': 'value',
52+
})
53+
54+
def test_missing_required_key_raises(self):
55+
"""Omitting a required key (subsysnqn or transport) must raise KeyError."""
56+
with self.assertRaises(KeyError):
57+
nvme.ctrl(self.ctx, {'transport': 'loop'})
58+
with self.assertRaises(KeyError):
59+
nvme.ctrl(self.ctx, {'subsysnqn': nvme.NVME_DISC_SUBSYS_NAME})
60+
61+
62+
if __name__ == '__main__':
63+
unittest.main()

0 commit comments

Comments
 (0)