classification
Title: 32 bit ctypes stdcall callback fails to restore stack pointer
Type: crash Stage: patch review
Components: ctypes, Windows Versions: Python 3.10, Python 3.9, Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: David Heffernan, christian.heimes, michaeldcurran, paul.moore, python-dev, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2019-11-08 16:57 by David Heffernan, last changed 2021-05-18 00:34 by python-dev.

Files
File name Uploaded Description Edit
py3.8crash.zip michaeldcurran, 2021-04-14 18:42 Testcase
Pull Requests
URL Status Linked Edit
PR 26204 open python-dev, 2021-05-18 00:34
Messages (8)
msg356249 - (view) Author: David Heffernan (David Heffernan) Date: 2019-11-08 16:57
Starting with Python 3.8 certain ctypes callbacks fail to restore the stack pointer.

In the repo below, when the DLL is compiled with MSVC under default debug settings, running the Python script leads to a debug error dialog which says:


Run-Time Check Failure #0 - The value of ESP was not properly saved across a function call.  This is usually a result of calling a function declared with one calling convention with a function pointer declared with a different calling convention.


It appears that when the C code calls the callback function, the value of ESP is 4 greater than it should be.

This problem does not occur with older versions of Python.


**DLL code**

#include <Windows.h>

BOOL APIENTRY DllMain(HMODULE hModule, DWORD ul_reason_for_call, LPVOID lpReserved)
{
    switch (ul_reason_for_call)
    {
    case DLL_PROCESS_ATTACH:
    case DLL_THREAD_ATTACH:
    case DLL_THREAD_DETACH:
    case DLL_PROCESS_DETACH:
        break;
    }
    return TRUE;
}

typedef void (__stdcall *MYCALLBACK)(int, double);

extern "C"
{
    __declspec(dllexport) void __stdcall foo(MYCALLBACK callback)
    {
        callback(1, 11);
        callback(2, 21);
        callback(3, 31);
    }
}

**Python code**

import ctypes
import ctypes.wintypes

def CallbackType(restype, *argtypes):

    def from_param(cls, obj):
        if obj is None:
            return obj
        return ctypes._CFuncPtr.from_param(obj)

    result = ctypes.WINFUNCTYPE(restype, *argtypes)
    result.from_param = classmethod(from_param)
    return result

MYCALLBACK = CallbackType(
    None,
    ctypes.c_int,
    ctypes.c_double
)

def callback(handle, time):
    print(handle, time)
mycallback = MYCALLBACK(callback)

lib = ctypes.WinDLL(r'path\to\dll\foo.dll')
func = getattr(lib, '_foo@4')
func.restype = None
func.argtypes = MYCALLBACK,
func(mycallback)
msg357345 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-11-22 23:49
I would guess this is due to the updated libffi, and probably it's a case that is not sufficiently tested.

Adding tests is the first step. It'll probably have to enumerate many parameter types (in case there's something weird here like misaligning the double after the int and not clearing it up, as opposed to a constant 4-byte offset).

Hopefully then the issue is on our side and not part of libffi, but it could be anywhere.
msg391095 - (view) Author: Michael Curran (michaeldcurran) * Date: 2021-04-14 18:42
I can also reproduce this. I will attach my own testcase below.
So far I see it when the callback is __stdcall (WINFUNCTYPE) and it takes an larger than 4 bytes (E.g. a long long or a VARIANT), with one or more arguments preceeding it such that this argument is not aligned on a multiple of 8 bytes. 
For example arguments can be:
* long, long long
* long, long, long, long long
But the corruption does not occur with something like:
* long, long, long long
My testcase uses long, long long to show the crash.
msg391096 - (view) Author: Michael Curran (michaeldcurran) * Date: 2021-04-14 18:47
This bug is reproduceable on both Python 3.8 and 3.9. But not 3.7.
Ths bug is seen in the real world, in the context of providing Python callbacks to Win32 API functions, or when implementing comtypes COM objects in Python.
For example, we see this crash in the NVDA screen reader project, in our implementation of ITTSBufNotifySink from the Microsoft SAPI4 speech API.
ITTSBufNotifySink::TextDataStarted takes a pointer (this), and a long long (qTimestamp).
https://github.com/nvaccess/nvda/issues/12152
msg391195 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-04-16 13:22
My previous post still stands. This requires a test and then (hopefully) a fix in ctypes or (potentially) a fix upstream in libffi.
msg391196 - (view) Author: David Heffernan (David Heffernan) Date: 2021-04-16 13:49
@Steve as a user of Python rather than a Python developer, I don't know what the process here. I understand the need for a test, and then a fix. And I would not be surprised if the act of fixing the issue led to a broadening of the test if the scope of the defect turns out to be wider than noted in the comments so far.

What is the process for a resolution to the issue being found. You've outlined the steps required, but how will they happen?
msg391197 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2021-04-16 13:54
It would help us if you or Michael could provide a minimal reproducer of the crash in form of a unit test and submit it as pull request.
msg391388 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-04-19 19:31
Even better, one that doesn't crash but safely returns a value that can be checked. (IIRC, we have a test that does this to ensure that structs smaller than 9 bytes are passed on the stack.)

Half C/half Python is fine - the C bits would go into _ctypes_test.c

Given how well you reported the issue in the first place, I'm confident the quick start in the devguide will get you up and building quickly: https://devguide.python.org/  Adding the test might be a bit more of an adventure (though not as much as fixing it...)
History
Date User Action Args
2021-05-18 00:34:03python-devsetkeywords: + patch
nosy: + python-dev

pull_requests: + pull_request24821
stage: test needed -> patch review
2021-04-19 19:31:47steve.dowersetmessages: + msg391388
2021-04-16 13:54:16christian.heimessetnosy: + christian.heimes

messages: + msg391197
versions: + Python 3.10
2021-04-16 13:49:45David Heffernansetmessages: + msg391196
2021-04-16 13:22:38steve.dowersetmessages: + msg391195
2021-04-14 18:47:57michaeldcurransetmessages: + msg391096
2021-04-14 18:42:02michaeldcurransetfiles: + py3.8crash.zip
nosy: + michaeldcurran
messages: + msg391095

2019-11-22 23:49:58steve.dowersetstage: test needed
messages: + msg357345
versions: + Python 3.9
2019-11-22 15:08:47eryksunsetnosy: + paul.moore, tim.golden, zach.ware, steve.dower
components: + Windows
2019-11-08 16:57:34David Heffernancreate