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

Incorrect function definitions #180

Closed
vincentisambart opened this issue Feb 24, 2021 · 5 comments · Fixed by #183
Closed

Incorrect function definitions #180

vincentisambart opened this issue Feb 24, 2021 · 5 comments · Fixed by #183

Comments

@vincentisambart
Copy link
Contributor

The function definitions for curl_easy_getinfo, curl_easy_setopt, curl_multi_setopt in lib/ethon/curls/functions.rb are incorrect.

They are declared in curl as follows.

CURL_EXTERN CURLcode curl_easy_getinfo(CURL *curl, CURLINFO info, ...);
CURL_EXTERN CURLcode curl_easy_setopt(CURL *curl, CURLoption option, ...);
CURL_EXTERN CURLMcode curl_multi_setopt(CURLM *multi_handle,
                                        CURLMoption option, ...);

Meaning that they should be declared something like:

attach_function :easy_getinfo, :curl_easy_getinfo, [:pointer, :info, :varargs], :easy_code
attach_function :easy_setopt, :curl_easy_setopt, [:pointer, :easy_option, :varargs], :easy_code
attach_function :multi_setopt, :curl_multi_setopt, [:pointer, :multi_option, :varargs], :multi_code

On some platforms, curl_easy_getinfo being declared as [:pointer, :info, :varargs], :easy_code, or (as it is currently in ethon) [:pointer, :info, :pointer], :easy_code, won't change how the method is called, but on some platforms (like on Apple Silicon / Apple M1), the ABI is different:

https://developer.apple.com/documentation/apple_silicon/addressing_architectural_differences_in_your_macos_code#3616882

The x86_64 and arm64 architectures have different calling conventions for variadic functions—functions with a variable number of parameters. On x86_64, the compiler treats fixed and variadic parameters the same, placing parameters in registers first and only using the stack when no more registers are available. On arm64, the compiler always places variadic parameters on the stack, regardless of whether registers are available. If you implement a function with fixed parameters, but redeclare it with variadic parameters, the mismatch causes unexpected behavior at runtime.

Fixing it on the ethon side is not that hard (quick and dirty patch here), but unfortunately that is not enough: it seems the ffi gem currently doesn't support passing callbacks via varargs (ffi/ffi#732).

So the ffi gem has to be fixed first.

@Kjarrigan
Copy link
Member

Thanks for the info. I'll check your links and this sounds like the issue that causes #172

@HamptonMakes
Copy link

This aligns perfectly with what my investigation into #172 was showing me!

@vincentisambart
Copy link
Contributor Author

Update: The fix to the ffi gem (ffi/ffi#885) got in its master branch
ethon should be able to make use of it when the next version of the ffi gem gets released.

After ffi's release, how to fix the ethon side probably depends on whether you want to continue supporting ffi <= 1.14.2 or not. If you want to continue supporting old versions of ffi, the simplest is probably to just modify lib/ethon/curls/functions.rb and change the definitions depending on FFI::VERSION (definitions for recent ffi would be probably similar to my quick and dirty patch). If you stop supporting older ffi, you would get more freedom for how you want to define the functions.
(I'm totally fine with writing a PR if needed, and you can freely take any parts of my patch as you want)

@Kjarrigan
Copy link
Member

@vincentisambart thanks for the update. If possible please provide an MR with the fix. Supporting older ffi version isn't necessary.

@vincentisambart
Copy link
Contributor Author

Created a PR: #183

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

Successfully merging a pull request may close this issue.

3 participants