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

Crash due to double free of OLE VARIANT data #3867

Closed
nvaccessAuto opened this Issue Feb 10, 2014 · 6 comments

Comments

Projects
None yet
3 participants
@nvaccessAuto

nvaccessAuto commented Feb 10, 2014

Reported by mdcurran on 2014-02-10 04:20
NVDA can crash when a ctypes callback wrapping a Python function returns, if the ctypes callback is declaired to have a VARIANT as an in parameter. In NVDA, so far this only happens for IUIAutomationPropertyChangedEventHandler::HandlePropertyChangedEvent.
Practically speaking, this is on Windows 8 when NVDA's log viewer is opened, and log level is set to input/output, and the user types keyboard input.
It has also been seen in windbg.
Though due to Windows OLE internal memory caching, the data in the richEdit field must be around 7000 characters or larger.

The problem is specifically a bug shared between comtypes's VARIANT implementation, and ctypes callbacks.
As ctypes does not convert a VARIANT to one of its simple Pythonic types (int, string etc), it creates a new VARIANT object and copies the existing stack memory into the VARIANT. But when the VARIANT python object is deleted, its del calls VariantClear, freeing the data the variant is holding. Yet, The caller of the callback, e.g. a Windows RPC stub, also calls VariantClear, thus causing the double free.
This will most likely will be rather tricky to fix in comtypes/ctypes as it shows up a limitation in the ctypes extention module itself, namely that a Python object can't tell if it was initialized for a callback or not.

For now, for any VARIANT given as an in parameter, we can manually set its vt member to VT_EMPTY within the callback, thus stopping the VariantClear on object deletion. Windows RPC or any other caller will still be able to call VariantClear on the original stack memory.

@nvaccessAuto

This comment has been minimized.

nvaccessAuto commented Feb 10, 2014

Comment 1 by Michael Curran <mick@... on 2014-02-10 04:31
In [f14f906]:

Provide a quick fix to a crash in NVDA due to a bug shared between comtypes VARIANT implementation and ctypes callbacks. Re #3867

Stops crashes in NVDA's log viewer and in windbg, when either contains a large amount of text and then its some how updated.

@nvaccessAuto

This comment has been minimized.

nvaccessAuto commented Feb 10, 2014

Comment 3 by mdcurran on 2014-02-10 04:45
Its possible to disable the OLE memory cache, which makes the crash show up no matter the length of the text in the richEdit control.
set the OANOCACHE environment variable to 1 to disable the cache.
This will cause the memory held by a BSTR to be really freed when SysFreeString is called.

@nvaccessAuto

This comment has been minimized.

nvaccessAuto commented Feb 10, 2014

Comment 5 by briang1 on 2014-02-10 10:21
For those with long but dodgy memories. I saw this and wondered if this had anything to do with the hang we had in XP when the UIA patch was used and some input fields were in use. In my case in Access.

@nvaccessAuto

This comment has been minimized.

nvaccessAuto commented Feb 10, 2014

Comment 6 by mdcurran (in reply to comment 5) on 2014-02-10 10:28
Replying to briang1:
I guess its not impossible. Once 2014.1 is released and next settles down again we may look at testing UIA on XP again now that this and other UIA support in NVDA has been fixed or changed somewhat since long ago.

@ehollig

This comment has been minimized.

Collaborator

ehollig commented Aug 21, 2017

@michaelDCurran, do you know what the stand is on this issue and whether it should be triaged?

@michaelDCurran

This comment has been minimized.

Contributor

michaelDCurran commented Aug 21, 2017

Closing as fixed as the only example was worked around. As long as the variant in a callback is set to vt_empty no crash will occur. The underlying bug is certainly in comtypes/ctypes. If we see this again we can res erect the effort and report it properly to both the Python and comtypes projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment