Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"a foreign function that will call a COM method" generated by ctypes.WINFUNCTYPE works in Python3.7, does not work as same in newer Python #99952

Closed
junkmd opened this issue Dec 2, 2022 · 10 comments
Assignees
Labels
3.10 only security fixes 3.11 only security fixes 3.12 bugs and security fixes topic-ctypes type-bug An unexpected behavior, bug, or error

Comments

@junkmd
Copy link
Contributor

junkmd commented Dec 2, 2022

Bug report

I'm contributing on enthought/comtypes.

In comtypes, there is a test for the behavior of Excel that is currently skipped. If I comment out the unittest.skip marker in that test, it works in Python 3.7 and fails in Python 3.11.

PS ...\comtypes> py -3.7 -m unittest comtypes.test.test_excel -vv
test (comtypes.test.test_excel.Test_EarlyBind) ... ok
test (comtypes.test.test_excel.Test_LateBind) ... ok
----------------------------------------------------------------------
Ran 2 tests in 10.576s
OK
PS ...\comtypes> py -3.7 -m clear_comtypes_cache -y  # <- clear caches, required!
Removed directory "...\comtypes\comtypes\gen"
Removed directory "...\AppData\Roaming\Python\Python37\comtypes_cache"
PS ...\comtypes> py -3.11 -m unittest comtypes.test.test_excel -vv       
test (comtypes.test.test_excel.Test_EarlyBind.test) ... FAIL
test (comtypes.test.test_excel.Test_LateBind.test) ... ok

======================================================================
FAIL: test (comtypes.test.test_excel.Test_EarlyBind.test)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "...\comtypes\comtypes\test\test_excel.py", line 62, in test
    self.assertEqual(xl.Range["A1:C3"].Value(),
AssertionError: Tuples differ: ((None, None, 10.0), ('x', 'y', 'z'), (3.0, 2.0, 1.0)) != ((10.0, 20.0, 31.4), ('x', 'y', 'z'), (3.0, 2.0, 1.0))

First differing element 0:
(None, None, 10.0)
(10.0, 20.0, 31.4)

- ((None, None, 10.0), ('x', 'y', 'z'), (3.0, 2.0, 1.0))
?   ------------

+ ((10.0, 20.0, 31.4), ('x', 'y', 'z'), (3.0, 2.0, 1.0))
?       ++++++++++++


----------------------------------------------------------------------
Ran 2 tests in 9.640s

FAILED (failures=1)

xl.Range[... calls a prototype-function generated by ctypes.WinFunctionType and ctypes.WINFUNCTYPE.
This is also reported in enthought/comtypes#212 and the test fails in Python 3.8 as well.
A strange callback behavior also occurs with simple COM libraries.
Therefore, I think that this may not be caused by the Excel specification.

There may be other regressions in ctypes callbacks that have also been reported in #82929.
Also, is #97513 a possible solution to this problem?

Any opinions would be appreciated.

Your environment

  • CPython versions tested on: Python 3.7.1 and Python 3.11.0
  • Windows 10

Linked PRs

@karpierz
Copy link

karpierz commented Dec 8, 2022

@michaelDCurran wrote:
For reference python/cpython#82929 has already been fixed in Python main.

Unfortunatelly the bug still exist in the newest (6.12.2022) versions of Python (3.10.9, 3.11.1).

For such code:

xl = CreateObject("Excel.Application")
from comtypes.gen.Excel import xlRangeValueDefault
xl.Workbooks.Add()
xl.Range["A1", "C1"].Value[xlRangeValueDefault] = (10, "20", 31.4)
print(xl.Range["A1", "C1"].Value[xlRangeValueDefault])
xl.Visible = True
input("Press a key")
xl.Quit()

we have results:

D:\Py>py -3.7 z_COM.py
((10.0, 20.0, 31.4),)
Press a key

D:\Py>py -3.10 z_COM.py
10.0
Press a key

D:\Py>py -3.11 z_COM.py
10.0
Press a key

PS: This is an important/real barrier to upgrading Python to versions newer than 3.7.

@michaelDCurran
Copy link
Contributor

michaelDCurran commented Dec 8, 2022 via email

@karpierz
Copy link

karpierz commented Dec 8, 2022

@michaelDCurran

Is the bug in the setting or the fetching of that value?

The same behaviour.

For such code:

xl = CreateObject("Excel.Application")
from comtypes.gen.Excel import xlRangeValueDefault
xl.Workbooks.Add()
xl.Range["A1", "C1"].Value[xlRangeValueDefault] = (10, "20", 31.4)
xl.Visible = True
print(xl.Range["A1", "C1"].Value[xlRangeValueDefault])
input("Press a key")  # in this place we just edited a row
print(xl.Range["A1", "C1"].Value[xlRangeValueDefault])
input("Press a key")
xl.Quit()

we have results:

D:\Py>py -3.7 z_COM.py
((10.0, 20.0, 31.4),)
Press a key
((11.0, 22.0, 33.0),)
Press a key

D:\Py>py -3.10 z_COM.py
10.0
Press a key
33.0
Press a key

D:\Py>py -3.11 z_COM.py
10.0
Press a key
33.0
Press a key

PS: py>3.7 gets last cell in the row instead of whole row.

@ynkdir
Copy link
Contributor

ynkdir commented Dec 9, 2022

VARIANT("A1") and VARIANT("C1") are created by from_param() for calling com method.
It seems that line 203 _VariantClear() is called after line 214 soon.
Then, memory is freed before calling com method.
Also, SysAllocStringLen() returns same memory address with "A1" for "C1".

comtypes/automation.py:
class tagVARIANT(Structure):
...
199     def __del__(self):
200         if self._b_needsfree_:
201             # XXX This does not work.  _b_needsfree_ is never
202             # set because the buffer is internal to the object.
203             _VariantClear(self)
204
205     def __repr__(self):
206         if self.vt & VT_BYREF:
207             return "VARIANT(vt=0x%x, byref(%r))" % (self.vt, self[0])
208         return "VARIANT(vt=0x%x, %r)" % (self.vt, self.value)
209
210     @classmethod
211     def from_param(cls, value):
212         if isinstance(value, cls):
213             return value
214         return cls(value)

@ynkdir
Copy link
Contributor

ynkdir commented Dec 10, 2022

Here is minimal reproducible code.

# fixed code for more specific.

import ctypes

# A is not ok

class A(ctypes.Structure):
    _fields_ = [("n1", ctypes.c_void_p), ("n2", ctypes.c_int)]

    def __del__(self):
        print("A.__del__", self.n1)

    @classmethod
    def from_param(cls, value):
        print("A.from_param:", value)
        return cls(n1=value)

ctypes.cdll.msvcrt.wprintf.argtypes = [ctypes.c_wchar_p, A]
print("A: start")
ctypes.cdll.msvcrt.wprintf("A: wprintf: %p\n", 1)
print("A: end")

# A: start
# A.from_param: 1
# A.__del__ 1
# A: wprintf: 00000032F1BEEBB0
# A: end

# B is ok

class B(ctypes.Structure):
    _fields_ = [("n1", ctypes.c_void_p)]

    def __del__(self):
        print("B.__del__", self.n1)

    @classmethod
    def from_param(cls, value):
        print("B.from_param:", value)
        return cls(n1=value)

ctypes.cdll.msvcrt.wprintf.argtypes = [ctypes.c_wchar_p, B]
print("B: start")
ctypes.cdll.msvcrt.wprintf("B: wprintf: %p\n", 2)
print("B: end")

# B: start
# B.from_param: 2
# B: wprintf: 0000000000000002
# B.__del__ 2
# B: end

@karpierz
Copy link

@ynkdir

199 def del(self):
200 if self.b_needsfree:
201 # XXX This does not work. b_needsfree is never
202 # set because the buffer is internal to the object.
203 _VariantClear(self)

Yes! I can confim. After commented:
pass #_VariantClear(self)

comtypes works as expected also for Py 3.8, 3.9, 3.10, 3.11

D:\Py>py -3.8 z_COM.py
10.0
Press a key
33.0
Press a key

C:\Python38\Lib\site-packages\comtypes>copy automation.py automation.py.org
        1 file(s) copied.

D:\Py>py -3.7 z_COM.py
((10.0, 20.0, 31.4),)
Press a key
((11.0, 22.0, 33.0),)
Press a key

D:\Py>py -3.8 z_COM.py
((10.0, 20.0, 31.4),)
Press a key
((11.0, 22.0, 33.0),)
Press a key

D:\Py>py -3.9 z_COM.py
((10.0, 20.0, 31.4),)
Press a key
((11.0, 22.0, 33.0),)
Press a key

D:\Py>py -3.10 z_COM.py
((10.0, 20.0, 31.4),)
Press a key
((11.0, 22.0, 33.0),)
Press a key

D:\Py>py -3.11 z_COM.py
((10.0, 20.0, 31.4),)
Press a key
((11.0, 22.0, 33.0),)
Press a key

Thanks,
Adam

@ynkdir
Copy link
Contributor

ynkdir commented Dec 10, 2022

Perhaps ctypes's StructParam_Type should keep object which copied from.
I am not familiar with python source.

diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c
index b9092d3981..55bcf044e8 100644
--- a/Modules/_ctypes/_ctypes.c
+++ b/Modules/_ctypes/_ctypes.c
@@ -412,6 +412,7 @@ _ctypes_alloc_format_string_with_shape(int ndim, const Py_ssize_t *shape,
 typedef struct {
     PyObject_HEAD
     void *ptr;
+    PyObject *keep;
 } StructParamObject;
 
 
@@ -419,6 +420,7 @@ static void
 StructParam_dealloc(PyObject *myself)
 {
     StructParamObject *self = (StructParamObject *)myself;
+    Py_XDECREF(self->keep);
     PyMem_Free(self->ptr);
     Py_TYPE(self)->tp_free(myself);
 }
@@ -466,6 +468,7 @@ StructUnionType_paramfunc(CDataObject *self)
 
         StructParamObject *struct_param = (StructParamObject *)obj;
         struct_param->ptr = ptr;
+        struct_param->keep = Py_NewRef(self);
     } else {
         ptr = self->b_ptr;
         obj = Py_NewRef(self);

@karpierz
Copy link

Yes. Seems like a main reason of this bug is a different object lifetime in Py <= 3.7, and Py >= 3.8
so (fortunately) in old Pythons this potential ctypes bug maybe was hidden/masked due to longer lifetime of referenced object?.

Maybe in comtypes will be useful/enough to use (as worarround/fix) pythonapi''s IncRef(obj)/DecRef(obj) ??
This will allow to avoid waiting for the official port and python releases with ev. fix?

from ctypes import py_object, pythonapi

Py_IncRef = pythonapi.Py_IncRef
Py_IncRef.argtypes = [py_object]
Py_IncRef.restype = None

Py_DecRef = pythonapi.Py_DecRef
Py_DecRef.argtypes = [py_object]
Py_DecRef.restype = None

PS: Unfortunately I am also not familiar with Python and ctypes C source code :(

@gpshead gpshead added 3.11 only security fixes 3.10 only security fixes 3.12 bugs and security fixes labels Jan 26, 2023
@gpshead gpshead self-assigned this Jan 26, 2023
gpshead pushed a commit that referenced this issue Jan 26, 2023
Fixes a reference counting issue with `ctypes.Structure` when a `from_param()` method call is used and the structure size is larger than a C pointer `sizeof(void*)`.

This problem existed for a very long time, but became more apparent in 3.8+ by change likely due to garbage collection cleanup timing changes.
gpshead pushed a commit to gpshead/cpython that referenced this issue Jan 26, 2023
…esult. (pythonGH-100169)

Fixes a reference counting issue with `ctypes.Structure` when a `from_param()` method call is used and the structure size is larger than a C pointer `sizeof(void*)`.

This problem existed for a very long time, but became more apparent in 3.8+ by change likely due to garbage collection cleanup timing changes..
(cherry picked from commit dfad678)

Co-authored-by: Yukihiro Nakadaira <yukihiro.nakadaira@gmail.com>
gpshead added a commit that referenced this issue Jan 26, 2023
… result (#101339)

[3.11] gh-99952: [ctypes] fix refcount issues in from_param() result. (GH-100169)

Fixes a reference counting issue with `ctypes.Structure` when a `from_param()` method call is used and the structure size is larger than a C pointer `sizeof(void*)`.

This problem existed for a very long time, but became more apparent in 3.8+ by change likely due to garbage collection cleanup timing changes..
(cherry picked from commit dfad678)

Co-authored-by: Yukihiro Nakadaira <yukihiro.nakadaira@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jan 26, 2023
…aram() result (pythonGH-101339)

[3.11] pythongh-99952: [ctypes] fix refcount issues in from_param() result. (pythonGH-100169)

Fixes a reference counting issue with `ctypes.Structure` when a `from_param()` method call is used and the structure size is larger than a C pointer `sizeof(void*)`.

This problem existed for a very long time, but became more apparent in 3.8+ by change likely due to garbage collection cleanup timing changes..
(cherry picked from commit dfad678)

(cherry picked from commit fa7c37a)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
Co-authored-by: Yukihiro Nakadaira <yukihiro.nakadaira@gmail.com>
mdboom pushed a commit to mdboom/cpython that referenced this issue Jan 31, 2023
…ython#100169)

Fixes a reference counting issue with `ctypes.Structure` when a `from_param()` method call is used and the structure size is larger than a C pointer `sizeof(void*)`.

This problem existed for a very long time, but became more apparent in 3.8+ by change likely due to garbage collection cleanup timing changes.
gpshead added a commit that referenced this issue Feb 4, 2023
…param() result (GH-101339) (#101340)

[3.11] gh-99952: [ctypes] fix refcount issues in from_param() result. (GH-100169)

Fixes a reference counting issue with `ctypes.Structure` when a `from_param()` method call is used and the structure size is larger than a C pointer `sizeof(void*)`.

This problem existed for a very long time, but became more apparent in 3.8+ by change likely due to garbage collection cleanup timing changes..
(cherry picked from commit dfad678)

(cherry picked from commit fa7c37a)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
Co-authored-by: Yukihiro Nakadaira <yukihiro.nakadaira@gmail.com>
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@zware
Copy link
Member

zware commented Mar 19, 2023

@junkmd, can you confirm that this is now fixed in the latest bugfix releases? I believe the merged PRs should have been included in 3.10.10 and 3.11.2.

@zware zware added the pending The issue will be closed if no feedback is provided label Mar 19, 2023
@junkmd
Copy link
Contributor Author

junkmd commented Mar 20, 2023

I have installed Python 3.10.10 and Python 3.11.2 and have run the comtypes tests that failed in the previous versions and have confirmed that they succeed(comtypes issue link).

Thank you.

@gpshead gpshead closed this as completed Mar 20, 2023
@zware zware removed the pending The issue will be closed if no feedback is provided label Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.10 only security fixes 3.11 only security fixes 3.12 bugs and security fixes topic-ctypes type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

7 participants