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

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

Open
jmp75 opened this issue Oct 29, 2024 · 8 comments

Comments

@jmp75
Copy link

jmp75 commented Oct 29, 2024

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
Copy link
Contributor

mattip commented Oct 29, 2024

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
Copy link
Author

jmp75 commented Oct 29, 2024

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
Copy link
Contributor

arigo commented Oct 29, 2024

The logic added in 9c2dfe8 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
Copy link
Contributor

arigo commented Oct 29, 2024

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
Copy link
Author

jmp75 commented Oct 29, 2024

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 added a commit to arigo/cffi that referenced this issue Oct 29, 2024
@arigo
Copy link
Contributor

arigo commented Oct 29, 2024

Can you try with these changes manually applied? c915708

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
Copy link
Author

jmp75 commented Oct 29, 2024

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

@mattip
Copy link
Contributor

mattip commented Oct 29, 2024

@jmp75 see #140.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants