python-cffi / cffi

A Foreign Function Interface package for calling C libraries from Python.
https://cffi.readthedocs.io/en/latest/
Other
127 stars 42 forks source link

old-style callback in Windows worked with cffi 1.16, crashes with cffi 1.17.1. #139

Open jmp75 opened 2 weeks ago

jmp75 commented 2 weeks ago

Context

A colleague reported an issue that I first reproduced as a 0xC0000005: Access violation executing location on some windows platforms. After a bit of diagnosing I noticed a change of implementation of dlopen for Windows in cffi=1.17, and the crash does not occur with cffi=1.16.

Also relates to this google group post

Repro

A repro is available from jmp75/py-cffi-callback-repro @ f7e1267. The program completes with cffi==1.16, but crashes with cffi==1.17.1 when activating the callback:

before registration
after registration
before has_callback_registered
after has_callback_registered
before triggering callback
...crash...

Observations

The change in the underlying implementation of dlopen for Windows may have been motivated by cffi/issues/64 , leading to the change Win32: pass the flags from dlopen() to LoadLibraryEx() #65.

dlopen under the hood in windows used to call return (void *)LoadLibraryA(filename); and it has now changed to return (void *)LoadLibraryExA(filename, NULL, flags);

I note I always passed flag(s) = 1, and note it seems to have been previously ignored on Windows.

mattip commented 2 weeks ago

I note I always passed flag(s) = 1

These lines seem wrong for windows

flags = int(os.environ.get("RT_CODE", 1))
ut_dll = ut_ffi.dlopen(str(native_lib_path), flags)  # Lazy loading

Looking at the meaning of 1 in the documentation it says

DONT_RESOLVE_DLL_REFERENCES If this value is used, and the executable module is a DLL, the system does not call DllMain for process and thread initialization and termination. Also, the system does not load additional executable modules that are referenced by the specified module. Note Do not use this value; it is provided only for backward compatibility. If you are planning to access only data or resources in the DLL, use LOAD_LIBRARY_AS_DATAFILE_EXCLUSIVE or LOAD_LIBRARY_AS_IMAGE_RESOURCE or both. Otherwise, load the library as a DLL or executable module using the LoadLibrary function.

On linux, 1 is RTLD_LAZY. Rather than use 1 explicitly, try ut_ffi.RTLD_LAZY which is not only more suggestive of what I think you are trying to do, but will work correctly. It evaluates to 1 on linux and 0 on windows.

Edit: ffi -> ut_ffi

jmp75 commented 2 weeks ago

Thank you; I was a bit puzzled by the flags on Windows.

Changing to

flags = ut_ffi.RTLD_LAZY
ut_dll = ut_ffi.dlopen(str(native_lib_path), flags)  # Lazy loading

then the loading process fails in def _load_backend_lib(backend, name, flags): with the error:

OSError: cannot load library 'test_native_library\build\Debug\test_native_library.dll': error 0x57.  Additionally, ctypes.util.find_library() did not manage to locate a library called 'test_native_library\\build\\Debug\\test_native_library.dll'

0x57 seems to be for ERROR_INVALID_PARAMETER according to https://learn.microsoft.com/en-us/windows/win32/debug/system-error-codes--0-499- .

arigo commented 2 weeks ago

The logic added in https://github.com/python-cffi/cffi/pull/65/commits/9c2dfe8aaa6fdd455bea4bb6b22f6be5dc12cabd is copied directly from ctypes. But for your example where you specify a relative path, it seems bogus. Does it work if you specify an absolute path, e.g. with os.path.abspath()?

arigo commented 2 weeks ago

Ah, modern ctypes does exactly that: they make the path absolute, in https://github.com/python/cpython/blob/dc2552d429c91310b0c410c3e856728d8866b05f/Lib/ctypes/__init__.py#L393

jmp75 commented 2 weeks ago

Spot on;

full_path = os.path.abspath(str(native_lib_path))
ut_dll = ut_ffi.dlopen(full_path, 0)

and now it passes even with cffi 1.17.1

So, my legacy hard coded flag 1 was ignored on windows previously when it was using LoadLibrary , but as @mattip points it mucks things up with the new backend call to the win32 api, hence the breakage. Thanks you both!

Thank

arigo commented 2 weeks ago

Can you try with these changes manually applied? https://github.com/python-cffi/cffi/commit/c915708910d7a5b3ce4661eaabb40adfce57374b

My github-fu is failing me and I'm not managing to make a pull request containing only this commit. If anyone else wants to do so, please do!

jmp75 commented 2 weeks ago

I will give a try tomorrow morning, see if I can make a PR. Thanks you

mattip commented 2 weeks ago

@jmp75 see #140.