diff --git a/comtypes/_safearray.py b/comtypes/_safearray.py index 08e50b444..b5284d154 100644 --- a/comtypes/_safearray.py +++ b/comtypes/_safearray.py @@ -98,6 +98,17 @@ def SafeArrayGetRecordInfo(pa: "_Pointer[tagSAFEARRAY]") -> "hints.IRecordInfo": return result # type: ignore +_SafeArrayCopy = _oleaut32.SafeArrayCopy +_SafeArrayCopy.restype = HRESULT +_SafeArrayCopy.argtypes = [POINTER(SAFEARRAY), POINTER(POINTER(SAFEARRAY))] + + +def SafeArrayCopy(pa: "_Pointer[SAFEARRAY]") -> "hints.LP_SAFEARRAY": + result = POINTER(SAFEARRAY)() + _SafeArrayCopy(pa, byref(result)) + return result # type: ignore + + SafeArrayGetElement = _oleaut32.SafeArrayGetElement SafeArrayGetElement.restype = HRESULT SafeArrayGetElement.argtypes = [POINTER(SAFEARRAY), POINTER(LONG), c_void_p] diff --git a/comtypes/automation.py b/comtypes/automation.py index da851a1f2..1372051f7 100644 --- a/comtypes/automation.py +++ b/comtypes/automation.py @@ -697,27 +697,22 @@ def _get_value(self, dynamic=False): except KeyError: pass - if self.vt & VT_RECORD: + if (self.vt & ~VT_ARRAY) == VT_RECORD: from comtypes.client import GetModule - from comtypes.typeinfo import IRecordInfo # Get generic safearray and its IRecordInfo - ri0 = cast(self._.pparray, POINTER(_safearray.SAFEARRAY)) - ri1 = _safearray.SafeArrayGetRecordInfo(ri0) - # QUESTION: Do we need to add a ref ? - ri2 = ri1.QueryInterface(IRecordInfo) + pa = cast(self._.pparray, POINTER(_safearray.SAFEARRAY)) + ri = _safearray.SafeArrayGetRecordInfo(pa) # Now containing typelib and through it the right class type - mod = GetModule(ri2.GetTypeInfo().GetContainingTypeLib()[0]) - ricls = getattr(mod, ri2.GetName()) + mod = GetModule(ri.GetTypeInfo().GetContainingTypeLib()[0]) + ricls = getattr(mod, ri.GetName()) # Cast the array to the right safearray(type), unpack -> return return cast(self._.pparray, _midlSAFEARRAY(ricls)).unpack() # Else we cannot unpack the array - raise NotImplementedError("typecode %d = 0x%x)" % (vt, vt)) - else: - raise NotImplementedError(f"typecode {vt} = 0x{vt:x})") + raise NotImplementedError(f"typecode {vt} = 0x{vt:x})") def __getitem__(self, index): if index != 0: diff --git a/comtypes/hints.pyi b/comtypes/hints.pyi index 7815740a1..ed26ee42d 100644 --- a/comtypes/hints.pyi +++ b/comtypes/hints.pyi @@ -62,7 +62,7 @@ class LP_SAFEARRAY(ctypes._Pointer[tagSAFEARRAY], Generic[_CT]): contents: tagSAFEARRAY _itemtype_: ClassVar[_CT] # type: ignore _vartype_: ClassVar[int] - _needsfree: ClassVar[bool] + _needsfree: bool @overload @classmethod diff --git a/comtypes/safearray.py b/comtypes/safearray.py index f99ce9d1c..9235f99b7 100644 --- a/comtypes/safearray.py +++ b/comtypes/safearray.py @@ -1,5 +1,6 @@ import array import threading +from collections.abc import Sequence from ctypes import POINTER, Structure, byref, c_long, cast, memmove, pointer, sizeof from typing import TYPE_CHECKING @@ -300,6 +301,8 @@ def _get_elements_raw(self, num_elements): the safearray.""" from comtypes.automation import VARIANT, tagCY, tagDEC + if self._vartype_ == VT_RECORD: + return self._get_records(num_elements) # XXX Not sure this is true: # For VT_UNKNOWN and VT_DISPATCH, we should retrieve the # interface iid by SafeArrayGetIID(). @@ -353,18 +356,68 @@ def _get_elements_raw(self, num_elements): return arr.copy() return ptr[:num_elements] - def keep_safearray(v): - # Simply keeping a reference to self does not keep the - # internal addresses of BSTR safe and strings will be - # overwritten. A copy of the bytes solves the problem - v1 = v.__class__() - memmove(byref(v1), byref(v), sizeof(v)) - return v1 - - return [keep_safearray(x) for x in ptr[:num_elements]] + # A Structure subtype without `_recordinfo_` cannot represent + # a valid COM SAFEARRAY of VT_RECORD: the COM specification + # requires IRecordInfo to describe the record layout. + # Reaching here means the type was constructed outside the + # normal COM codegen path. No safe extraction strategy + # exists, so raise rather than silently returning corrupt data. + raise NotImplementedError( + f"{self._itemtype_!r} is a Structure without" + " _recordinfo_; safe extraction from SAFEARRAY is not" + " supported." + ) finally: _safearray.SafeArrayUnaccessData(self) + def _get_records(self, num_elements: int) -> Sequence[Structure]: + """Extract element records as a flat list while avoiding double-free + crashes. + + The source `SAFEARRAY` may be destroyed at any time by COM. + Without proper lifetime management, returned elements would suffer + use-after-free, or the array would double-free when its Python + instance is finalized. + + We avoid this by deep-copying the `SAFEARRAY`. The returned + (copied) element record instances each hold a reference to the + copied array instance, keeping the copied array alive until all + returned element records are gone. + + Once the array and all element records are GC'd in Python, the + copied array is automatically `SafeArrayDestroy`ed, which destroys + the array descriptor and all data: `IRecordInfo::RecordClear` on + records. + """ + ptr = POINTER(self._itemtype_)() # container for the values + # Make an independent `SAFEARRAY` so its data does not depend on + # the lifetime of the source `SAFEARRAY`, which is `self`. + copied = _safearray.SafeArrayCopy(self) # type: ignore + # Setting this flag makes `__del__` call `SafeArrayDestroy` + # when the Python wrapper is finalized. + copied._needsfree = True + # Lock the copied `SAFEARRAY` and get its native record storage. + # A direct `ptr` avoids making a separately owned record copy. + _safearray.SafeArrayAccessData(copied, byref(ptr)) + try: + + def keep_safearray(v): + v1 = v.__class__() + # Copy bytes, not pointed-to data; `copied` owns that data. + memmove(byref(v1), byref(v), sizeof(v)) + # This strong reference keeps `copied` alive while `v1` + # is alive, so `copied.__del__` cannot run and destroy + # the SAFEARRAY. A weak reference would not do this. + v1.__keepref = copied + return v1 + + # Return only the requested number of records. + # Each result keeps the `copied` wrapper alive. + return [keep_safearray(x) for x in ptr[:num_elements]] + finally: + # Unlock the native SAFEARRAY storage; this does not destroy it. + _safearray.SafeArrayUnaccessData(copied) + def _get_row(self, dim, indices, lowerbounds, upperbounds): # loop over the index of dimension 'dim' # we have to restore the index of the dimension we're looping over diff --git a/comtypes/test/test_record_safearray_bstr.py b/comtypes/test/test_record_safearray_bstr.py index 4d8dff488..aa475dce0 100644 --- a/comtypes/test/test_record_safearray_bstr.py +++ b/comtypes/test/test_record_safearray_bstr.py @@ -8,8 +8,6 @@ import gc import io import multiprocessing -import re -import sys import tempfile import unittest from _ctypes import COMError @@ -393,7 +391,6 @@ def test_first_record_empty_safearray_by_create(self): del records gc.collect() - @unittest.skipIf(sys.version_info >= (3, 14), "CRASH! CHARACTERIZATION(#63)") def test_reverse_records_length_3(self): ifc = _create_server(self.interface) @@ -474,25 +471,9 @@ def call_reverse_bstr_records(tmpfile: Path) -> None: @unittest.skipIf(IMPORT_FAILED, "This depends on the out of process COM-server.") class TestOwnership(unittest.TestCase): - @unittest.skipIf(sys.version_info < (3, 14), "NOT CRASH! CHARACTERIZATION(#63)") def test_bstr_record_safearray_ownership(self): exitcode, output = run_in_mp(call_reverse_bstr_records) - self.assertIsNotNone(exitcode) - # Python's process.exitcode on Windows may return the exit code as - # either a signed or unsigned 32-bit integer. Masking with 0xFFFFFFFF - # normalizes it to the unsigned 32-bit representation for comparison - # with the NTSTATUS constants defined above. - self.assertEqual( - exitcode & 0xFFFFFFFF, # type: ignore - STATUS_HEAP_CORRUPTION, - msg="CHARACTERIZATION(#63): " - "If https://github.com/enthought/comtypes/issues/63 is fixed, " - "this failure is expected; change it to a normal-path test.", - ) - ptn = re.compile( - r'test_record_safearray_bstr\.py", line \d+ in call_reverse_bstr_records' - ) - self.assertTrue(bool(ptn.search(output))) + self.assertEqual(exitcode, 0, msg=f"Process crashed with output: {output}") if __name__ == "__main__":