Passing structs by value on the stack is incorrect on arm64

[rfk]

The version of libffi bundled with JNA appears to incorrectly pass structs on the stack on arm64 devices.

Consider a function that takes struct arguments by value, like this example which just returns a field from its ninth struct argument:

typedef struct WrappedLong {
    int64_t inner;
} WrappedLong;

int64_t return_the_ninth_argument(WrappedLong arg1, WrappedLong arg2, WrappedLong arg3, WrappedLong arg4, WrappedLong arg5, WrappedLong arg6, WrappedLong arg7, WrappedLong arg8, WrappedLong arg9) {
  return arg9.inner;
}

When I try to call this function via JNA on an arm64 device, is returns a large and random-looking integer rather than the value passed in the ninth argument. I've published a minimal example to show how I'm calling this code via JNA, since the declarations seem like they would be a bit unwieldy copy-pasted into a github comment:

• https://github.com/rfk/arm64-jna-structbug/blob/2b6f8ffe618c4c40a21f79c0e103e8f7000d5734/src/main/java/MainActivity.kt#L88

The linked file is part of a tiny Android application that I have been using to investigate this issue and which can be used to reproduce it in full: rfk/arm64-jna-structbug.

Running this example app works as expected on an x86_64 device, and it works as expected on an arm64 device if I modify it to return e.g. the eight argument rather than the ninth.

I believe this to be a bug in the libffi bundled with JNA:

• The first 8 arguments to this function are passed in registers, but subsequent arguments are passed on the stack.
• The code for passing small structs on the stack appears to copy a pointer to the argument value rather than copying the argument value itself.

If I change the linked line of code to memcpy from ecif->avalue[i] instead of ecif->avalue + i then my example app seems to work as expected, correctly returning the value passed as the ninth argument.

I took a quick look in the equivalent code in upstream libffi and the issue seems to be fixed there, although as far as I can tell it was fixed as a side-effect of this significant refactor/cleanup, so I'm not sure if it was ever reported upstream as a bug.

Since upstream libffi seems to have diverged quite a bit from the version in this repo, I wanted to ask for advice before proceeding any further here. Would this be best approached by trying to update the bundled version of libffi, or as a standalone fix?

For completeness: I reproduced this issue on a Samsung Galaxy S8 running Android 9, which is an arm64-v8a device, using both the released JNA 5.6.0 and a local build of the latest JNA master from github.

[matthiasblaesing]

Thank you for your analysis. To address the core question:

Since upstream libffi seems to have diverged quite a bit from the version in this repo, I wanted to ask for advice before proceeding any further here. Would this be best approached by trying to update the bundled version of libffi, or as a standalone fix?

I suspect, that with windows entering new territory (arm64) and apple also jumping that wagon, updates in libffi are to be expected (at least in the build system part), which we will also need. I don't see benefit in doing/repeating FFI upstream work here in the repository. What is more, JNA can already be build against a system installed libffi, so I'd consider updating the bundled version to be save.

There are instructions for updating the bundled libffi in native/README.libffi. This reads as updates are expected.

@twall anything from your point, that would prevent updating to a new upstream version of libff?

[twall]

There's no particular barrier to pulling in a libffi update. Most likely some of the Make targets will need tweaking if there have been changes to the libffi build structure. In the past, libffi was updated _very_ infrequently (every 3-4 years or so); usually an update was prompted by some specific bug or feature requirement (as the arm64 seems to be here). I wouldn't necessarily bother with rebuilding the natives for the non-arm platforms unless it were a) easy to do or b) the new libffi release was bringing some important fixes. T/

…

On Sun, Nov 1, 2020 at 3:46 PM Matthias Bläsing ***@***.***> wrote: Thank you for your analysis. To address the core question: Since upstream libffi seems to have diverged quite a bit from the version in this repo, I wanted to ask for advice before proceeding any further here. Would this be best approached by trying to update the bundled version of libffi, or as a standalone fix? I suspect, that with windows entering new territory (arm64) and apple also jumping that wagon, updates in libffi are to be expected (at least in the build system part), which we will also need. I don't see benefit in doing/repeating FFI upstream work here in the repository. What is more, JNA can already be build against a system installed libffi, so I'd consider updating the bundled version to be save. There are instructions for updating the bundled libffi in native/README.libffi. This reads as updates are expected. @twall <https://github.com/twall> anything from your point, that would prevent updating to a new upstream version of libff? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1259 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFYZLJQ7QQRN5T6S5MGJXDSNXCKHANCNFSM4S67KT7Q> .

[twall]

Just note that there may be some local libffi tweaks that need to be preserved. I've posted patches for those upstream, but I don't know if they ever got adopted (I recall one in particular had to do with stdcall handling). It should be straightforward to diff these against the last libfffi import. T.

…

On Sun, Nov 1, 2020 at 4:35 PM Timothy Wall ***@***.***> wrote: There's no particular barrier to pulling in a libffi update. Most likely some of the Make targets will need tweaking if there have been changes to the libffi build structure. In the past, libffi was updated _very_ infrequently (every 3-4 years or so); usually an update was prompted by some specific bug or feature requirement (as the arm64 seems to be here). I wouldn't necessarily bother with rebuilding the natives for the non-arm platforms unless it were a) easy to do or b) the new libffi release was bringing some important fixes. T/ On Sun, Nov 1, 2020 at 3:46 PM Matthias Bläsing ***@***.***> wrote: > Thank you for your analysis. To address the core question: > > Since upstream libffi seems to have diverged quite a bit from the version > in this repo, I wanted to ask for advice before proceeding any further > here. Would this be best approached by trying to update the bundled version > of libffi, or as a standalone fix? > > I suspect, that with windows entering new territory (arm64) and apple > also jumping that wagon, updates in libffi are to be expected (at least in > the build system part), which we will also need. I don't see benefit in > doing/repeating FFI upstream work here in the repository. What is more, JNA > can already be build against a system installed libffi, so I'd consider > updating the bundled version to be save. > > There are instructions for updating the bundled libffi in > native/README.libffi. This reads as updates are expected. > > @twall <https://github.com/twall> anything from your point, that would > prevent updating to a new upstream version of libff? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#1259 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAFYZLJQ7QQRN5T6S5MGJXDSNXCKHANCNFSM4S67KT7Q> > . >

[matthiasblaesing]

I traced through the updates of native libffi and found these commits, that touched the imported code and thus would need to be checked:

The code at 951de1a is mostly in sync with 3.2.1, around that commit I found these:

• a3afbfe Revert incorrect bitwise size check; sizes can not be guaranteed to be aligned or padded, so check small struct sizes explicitly
• 2577b95 fix mingw struct return when using SYSV ABI, fix msvcc stdcall closures
• 5ce1c84 fix return type jump tables to accommodate addition of FFI_TYPE_COMPLEX
• 52eed7b Fix windows builds under MSVC, 32- and 64-bit
• 4e8c935 Apply input from @joshtriplett on more general stdcall handling
• 4836f05 don't align arguments in stdcall
• ec16947 update w32ce-arm build, native
• 1f1972b remove msvc90 dependency, spurious '-g' flags in libffi, fix compiler warnings against dlmalloc.c

A diff that could only be traced into the 3.2.1 merge was:

diff -urb ../../libffi/src/x86/ffi.c libffi/src/x86/ffi.c
--- ../../libffi/src/x86/ffi.c	2020-11-02 18:35:07.606547408 +0100
+++ libffi/src/x86/ffi.c	2020-11-02 18:42:52.212778456 +0100
@@ -329,6 +329,7 @@
 #ifndef X86_WIN64
   if (cif->abi == FFI_SYSV || cif->abi == FFI_UNIX64)
 #endif
+  if (cif->abi != FFI_STDCALL)
     cif->bytes = (cif->bytes + 15) & ~0xF;
 #endif

[twall]

You might be able to set libffi repo as an additional remote and merge libffi updates locally. I recall I had a git subtree setup so that I could do just that.

…

On Mon, Nov 2, 2020 at 1:05 PM Matthias Bläsing ***@***.***> wrote: I traced through the updates of native libffi and found these commits, that touched the imported code and thus would need to be checked: The code at 951de1a <951de1a> is mostly in sync with 3.2.1, around that commit I found these: - a3afbfe <a3afbfe> Revert incorrect bitwise size check; sizes can not be guaranteed to be aligned or padded, so check small struct sizes explicitly - 2577b95 <2577b95> fix mingw struct return when using SYSV ABI, fix msvcc stdcall closures - 5ce1c84 <5ce1c84> fix return type jump tables to accommodate addition of FFI_TYPE_COMPLEX - 52eed7b <52eed7b> Fix windows builds under MSVC, 32- and 64-bit - 4e8c935 <4e8c935> Apply input from @joshtriplett <https://github.com/joshtriplett> on more general stdcall handling - 4836f05 <4836f05> don't align arguments in stdcall - ec16947 <ec16947> update w32ce-arm build, native - 1f1972b <1f1972b> remove msvc90 dependency, spurious '-g' flags in libffi, fix compiler warnings against dlmalloc.c A diff that could only be traced into the 3.2.1 merge was: diff -urb ../../libffi/src/x86/ffi.c libffi/src/x86/ffi.c--- ../../libffi/src/x86/ffi.c 2020-11-02 18:35:07.606547408 +0100+++ libffi/src/x86/ffi.c 2020-11-02 18:42:52.212778456 +0100@@ -329,6 +329,7 @@ #ifndef X86_WIN64 if (cif->abi == FFI_SYSV || cif->abi == FFI_UNIX64) #endif+ if (cif->abi != FFI_STDCALL) cif->bytes = (cif->bytes + 15) & ~0xF; #endif — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1259 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFYZLJKYXPXTM2L5BEHWRLSN3YD3ANCNFSM4S67KT7Q> .

[matthiasblaesing]

You might be able to set libffi repo as an additional remote and merge libffi updates locally. I recall I had a git subtree setup so that I could do just that.

It is even documented: native/README.libffi. I would use the upstream repository of libffi as the base, but otherwise this might work. The big question in the end is: Is it easier to check if all JNA patches have been integrated upstream and switch to vanialla libffi or is it easier to solve merge conflicts.

[twall]

On Tue, Nov 3, 2020 at 1:02 PM Matthias Bläsing ***@***.***> wrote: You might be able to set libffi repo as an additional remote and merge libffi updates locally. I recall I had a git subtree setup so that I could do just that. It is even documented: native/README.libffi.

I guess it pays to write things down for future reference :)

I would use the upstream repository of libffi as the base, but otherwise this might work. The big question in the end is: Is it easier to check if all JNA patches have been integrated upstream and switch to vanialla libffi or is it easier to solve merge conflicts.

I doubt you'll get many merge conflicts unless a module were completely rewritten, and even then you've got the few commits to isolate the actual changes to be (re-)applied. I chose to replicate the repo since time between libffi releases was very long, and time to integrate submitted bugfixes possibly even longer. I also sometimes needed to build debug versions for validation I couldn't get via libffi tests. T.

…

[dkocher]

Relates to #1238.

[tresf]

can we build from the embedded libffi source/can we update it?

Edit: Sorry, I had missed the native/README.libffi which has explicit instructions for bumping the subtree. According to conversation in #1259, there's divergence issues so I'm still a bit confused as to how best to tackle this.

Originally posted by @tresf in #1238 (comment)

I doubt you'll get many merge conflicts unless a module were completely rewritten, and even then you've got the few commits to isolate the actual changes to be (re-)applied.

I tried this. I added upstream libffi/libffi's master, did the pull with squash per README and I received > 90 conflicts. Am I doing something wrong, or should I revert back to the brute-force replacement used in #1264 while making sure to cherry-pick the commits identified atop our directory?

I'm also willing to fork, point, and submodule, I have a bit of experience with this, but it doesn't come without consequences.

[twall]

can you generalize at all about the nature of the conflicts? that's way more than I would have expected.

…

On Thu, Nov 12, 2020 at 5:06 PM Timothy Wall ***@***.***> wrote: can you generalize at all about the nature of the conflicts? that's way more than I would have expected. On Thu, Nov 12, 2020 at 4:52 PM Tres Finocchiaro ***@***.***> wrote: > can we build from the embedded libffi source/can we update it? > > *Edit:* Sorry, I had missed the native/README.libffi which has explicit > instructions for bumping the subtree. According to conversation in #1259 > <#1259>, there's > divergence issues so I'm still a bit confused as to how best to tackle this. > > *Originally posted by @tresf <https://github.com/tresf> in #1238 > (comment) > <#1238 (comment)>* > > I doubt you'll get many merge conflicts unless a module were completely > rewritten, and even then you've got the few commits to isolate the actual > changes to be (re-)applied. > > I tried this. I added upstream libffi/libffi's master, did the pull with > squash per README and I received > 90 conflicts. Am I doing something > wrong, or should I revert back to cherry-picking the commits identified > <#1259 (comment)> > atop our directory? > > I'm also willing to fork, point, and submodule, I have a bit of > experience with this <https://tinyurl.com/y6oksb4r>, but it doesn't come > without consequences > ***@***.***/sharing-code-and-why-git-submodules-is-a-bad-idea-1efd24eb645d> > . > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#1259 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAFYZLIBFLKMT4YQU5JEE2TSPRKHFANCNFSM4S67KT7Q> > . >

[tresf]

I started evaluating the commits linked above but it appears libffi has changed too much for nearly all commits listed, so my evaluation isn't very comprehensive.

Click to expand

Commit	Description	Status
a3afbfe	Revert incorrect bitwise size check; sizes can not be guaranteed to be aligned or padded, so check small struct sizes explicitly	❓: x86/ffi.c has completely changed.
2577b95	fix mingw struct return when using SYSV ABI, fix msvcc stdcall closures	❓ src/x86/win32.S is missing.
5ce1c84	fix return type jump tables to accommodate addition of FFI_TYPE_COMPLEX	❓ src/x86/win32.S is missing, src/x86/win64.S doesn't match.
52eed7b	Fix windows builds under MSVC, 32- and 64-bit	❓ ffitarget.h has the patched. Most other files have completely changed.
4e8c935	Apply input from @joshtriplett on more general stdcall handling	❓: x86/ffi.c has completely changed although similar 64-bit guards seem to be present throughout.
4836f05	don't align arguments in stdcall	❓ x86/ffi.c has completely changed. The X86_WIN32| FFI_STDCALL guard seems to be missing, but it may be done elsewhere.
ec16947	update w32ce-arm build, native	❓ Files have completely changed.
1f1972b	remove msvc90 dependency, spurious '-g' flags in libffi, fix compiler warnings against dlmalloc.c	❓ Makefile changes seem OK. dlmalloc.c fixes can't easily be determined.

A diff that could only be traced into the 3.2.1 merge was: (x86/ffi.c's [X86_WIN32| FFI_STDCALL] from 4836f05.)

I noticed this too however the code has changed so much, we'd need someone much more familiar with it to know if the patch is needed or exists elsewhere. Here's what it looks like upstream...

jna/native/libffi/src/x86/ffi.c

Lines 180 to 187 in 0ae7234

	for (i = 0, n = cif->nargs; i < n; i++)
	{
	ffi_type *t = cif->arg_types[i];
	bytes = FFI_ALIGN (bytes, t->alignment);
	bytes += FFI_ALIGN (t->size, FFI_SIZEOF_ARG);
	}
	cif->bytes = bytes;

can you generalize at all about the nature of the conflicts? that's way more than I would have expected.

Hard to put into words, but a lot goes into conflict, so perhaps I'm doing it wrong? Even the .gitignore and .travis.yml files went into conflict, which I wouldn't expect us to touch: https://gist.github.com/tresf/fb4034ccc75cb55eb23dd43a3cea3cb1

[rfk]

I saw that the JNA 5.7 release includes an update the libffi, so I just tried it with my testcase and am no longer able to reproduce the bug. Thanks folks! I think this can be safely closed.

[dbwiddis]

Thanks for the update! Closing per your report.