Repository navigation
Fix SAFEARRAY ownership for VT_ARRAY | VT_RECORD values. - #984
Merged
junkmd merged 8 commits intoNov 5, 2026
Merged
Conversation
…stead of crash characterization.
It is an instance attribute, not a class attribute. Each `SAFEARRAY` pointer instance has its own `_needsfree` flag to track memory ownership.
Add helper `_is_recordinfo_guids` and type alias `_RIGuids` to check if a type is a `Structure` with valid `_recordinfo_` class variable.
… handle record extraction.
Problem: - `self.vt & VT_RECORD` is wrong because `VT_RECORD` (36) is a value, not a flag. - Bitwise AND with `VT_RECORD` can be true for unrelated vt like `VT_INT` (22). Solution: - Change the check to `(self.vt & ~VT_ARRAY) == VT_RECORD`. - This correctly checks if the base type (without `VT_ARRAY` flag) is `VT_RECORD`. - Remove redundant code and simplify the logic.
…for `Structure` without `_recordinfo_`. After adding the `is_struct_type_with_recordinfo` early-return in `_get_elements_raw`, the else branch reached by a `Structure` subtype without `_recordinfo_` could no longer be handled correctly by the remaining `memmove`-based `keep_safearray`. A `Structure` `SAFEARRAY` that lacks `IRecordInfo` is not constructable through normal COM codegen: the COM spec requires `IRecordInfo` for any `VT_RECORD` array. Silently returning data from a shallow byte-copy risks double-free crashes for fields with `BSTR` or pointer semantics. Raising `NotImplementedError` makes the unsupported state explicit instead of returning potentially corrupt data, and signals to future maintainers that a safe extraction path needs to be designed if this case ever becomes real.
In `_get_elements_raw`, use `self._vartype_ == VT_RECORD` instead of `is_struct_type_with_recordinfo(self._itemtype_)`. The `_vartype_` is set to `VT_RECORD` when the item type is a `Structure` with valid `_recordinfo_`. This check is done in `_make_safearray_type`. Benefits: - No function call needed - No import from typeinfo needed - Simple integer comparison is faster - Code is shorter and clearer Also remove unused `is_struct_type_with_recordinfo` and `_is_recordinfo_guids` from `typeinfo.py` because they are not used anywhere now.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## record_safearray_characterization_tests #984 +/- ##
========================================================================
Coverage 90.72% 90.73%
========================================================================
Files 147 147
Lines 15344 15353 +9
========================================================================
+ Hits 13921 13930 +9
Misses 1423 1423 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Related to #63
Summary
This change makes record arrays safer to unpack, resolves the correct record type, and manages the lifetime of the
SAFEARRAYused by the unpacked records.Changes
The previous implementation (#104) could unpack record arrays with a shallow memory copy. This is not safe (at least for Python 3.14 and later,) for records containing
BSTRvalues or other managed resources.This change gives the unpacked records their own copy of the
SAFEARRAYand keeps that copy alive for as long as the records need it.On Python 3.14 and later, the related operation no longer crashes. This may be because garbage collection behavior has changed, even in builds without free-threading, and the release order is now different from earlier Python versions.
The updated test checks that the operation completes successfully, which confirms this behavior.