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

Avoid modifying shared DLL objects #735

Closed
moi15moi opened this issue Jan 7, 2025 · 12 comments · Fixed by #737
Closed

Avoid modifying shared DLL objects #735

moi15moi opened this issue Jan 7, 2025 · 12 comments · Fixed by #737
Labels
good first issue Good for newcomers
Milestone

Comments

@moi15moi
Copy link
Contributor

moi15moi commented Jan 7, 2025

Currently, comtypes modifies shared DLL objects. Below are examples where such modifications occur:

As noted in this comment, it is preferable to use ctypes.WinDLL instead of ctypes.windll. This approach avoids modifying shared DLL objects and ensures better isolation.

@junkmd junkmd added the good first issue Good for newcomers label Jan 8, 2025
@junkmd
Copy link
Collaborator

junkmd commented Jan 8, 2025

+1.

Feel free to open PRs.

moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
Partially fix enthought#735

Also, changed IUnknown to ICreateTypeLib2 because CreateTypeLib2 doesn't take IUnknown as a parameter
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
Partially fix enthought#735

I needed to use a try except because, now, _QueryPathOfRegTypeLib return a HRESULT and not a c_int
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 12, 2025
Partially fix enthought#735

Also, I removed the check to detect if LoadTypeLibEx exist. It is available since Windows 2000, so there isn't any reason to check for it's existence
moi15moi added a commit to moi15moi/comtypes that referenced this issue Jan 13, 2025
junkmd pushed a commit that referenced this issue Jan 13, 2025
#737)

* [server\register.py] Use WinDLL instead of windll

Partially fix #735

* [server\register.py] Replace * import with specific imports

- Prefer `from import` over `import`
- Use LPCWSTR instead of c_wchar_p.
- Don't directly import `comtypes.server.localserver` in a function.
@junkmd junkmd reopened this Jan 13, 2025
@junkmd
Copy link
Collaborator

junkmd commented Jan 26, 2025

  • bstr.py
    Note that I don't really know how we can do it because if I put this code before the definition of the class, BSTR doesn't exist and if I put it after, _SysFreeString in _free: Callable[["BSTR"], Any] isn't defined:

Have you tried this approach?

+_oleaut32 = WinDLL("oleaut32")
+
+_SysFreeString = _oleaut32.SysFreeString
+
+
 class BSTR(_SimpleCData):
     """The windows BSTR data type"""

@@ -18,9 +23,7 @@ class BSTR(_SimpleCData):
         self._needsfree = True
         return self.value

-    def __del__(
-        self, _free: Callable[["BSTR"], Any] = windll.oleaut32.SysFreeString
-    ) -> None:
+    def __del__(self, _free: Callable[["BSTR"], Any] = _SysFreeString) -> None:
         # Free the string if self owns the memory
         # or if instructed by __ctypes_from_outparam__.
         if self._b_base_ is None or self._needsfree:
@@ -35,3 +38,7 @@ class BSTR(_SimpleCData):
         # right thing, it doesn't ensure that SysFreeString is called
         # on destruction.
         return cls(value)
+
+
+_SysFreeString.argtypes = [BSTR]
+_SysFreeString.restype = None

@junkmd
Copy link
Collaborator

junkmd commented Feb 2, 2025

The work on the remaining files has been completed, and with this, shared DLL objects such as windll.libraryname and oledll.libraryname have been removed from this project.

Below is the result of a Git command to confirm this.

PS ...\comtypes> git grep -n -C 2 "windll" -- @(git ls-files)
comtypes/_comobject.py-450-
comtypes/_comobject.py-451-    def IDispatch_GetIDsOfNames(self, this, riid, rgszNames, cNames, lcid, rgDispId):
comtypes/_comobject.py:452:        # This call uses windll instead of oledll so that a failed
comtypes/_comobject.py-453-        # call to DispGetIDsOfNames will return a HRESULT instead of
comtypes/_comobject.py-454-        # raising an error.
--
comtypes/_comobject.py-483-                # values, without going through GetIDsOfNames first.
comtypes/_comobject.py-484-                return hresult.DISP_E_MEMBERNOTFOUND
comtypes/_comobject.py:485:            # This call uses windll instead of oledll so that a failed
comtypes/_comobject.py-486-            # call to DispInvoke will return a HRESULT instead of raising
comtypes/_comobject.py-487-            # an error.
--
comtypes/tools/tlbparser.py-1-import os
comtypes/tools/tlbparser.py-2-import sys
comtypes/tools/tlbparser.py:3:from ctypes import alignment, c_void_p, sizeof, windll
comtypes/tools/tlbparser.py-4-from typing import Any, Dict, List, Optional, Tuple
comtypes/tools/tlbparser.py-5-
--
comtypes/tools/tlbparser.py-750-        # workaround Windows 7 bug in QueryPathOfRegTypeLib returning relative path
comtypes/tools/tlbparser.py-751-        try:
comtypes/tools/tlbparser.py:752:            dll = windll.LoadLibrary(full_filename)
comtypes/tools/tlbparser.py-753-            full_filename = _get_module_filename(dll._handle)
comtypes/tools/tlbparser.py-754-            del dll
PS ...\comtypes> git grep -n -C 2 "oledll" -- @(git ls-files)
comtypes/_comobject.py-450-
comtypes/_comobject.py-451-    def IDispatch_GetIDsOfNames(self, this, riid, rgszNames, cNames, lcid, rgDispId):
comtypes/_comobject.py:452:        # This call uses windll instead of oledll so that a failed
comtypes/_comobject.py-453-        # call to DispGetIDsOfNames will return a HRESULT instead of
comtypes/_comobject.py-454-        # raising an error.
--
comtypes/_comobject.py-483-                # values, without going through GetIDsOfNames first.
comtypes/_comobject.py-484-                return hresult.DISP_E_MEMBERNOTFOUND
comtypes/_comobject.py:485:            # This call uses windll instead of oledll so that a failed
comtypes/_comobject.py-486-            # call to DispInvoke will return a HRESULT instead of raising
comtypes/_comobject.py-487-            # an error.

Thank you for handling the large code changes and for splitting the modifications to make reviewing easier.

@moi15moi
Copy link
Contributor Author

moi15moi commented Feb 2, 2025

Perfect, is there any task left before we can close this issue?

Do you have in mind to switch all the OleDLL to WinDLL?

@junkmd
Copy link
Collaborator

junkmd commented Feb 2, 2025

There shouldn’t be anything left for us to do on this issue.

Feel free to close it.

@junkmd junkmd added this to the 1.4.10 milestone Feb 2, 2025
@moi15moi moi15moi closed this as completed Feb 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants