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

Fix variadic arguments on arm64 darwin ABI #577

Closed
wants to merge 1 commit into from

Conversation

glandium
Copy link
Contributor

Normal arguments that spill on the stack are packed, but not variadic
arguments. This is handled correctly for their placement already, but
code generated on the callee side with va_list expects word-size
sign-extension, so we need to fill the entire word.

Normal arguments that spill on the stack are packed, but not variadic
arguments. This is handled correctly for their placement already, but
code generated on the callee side with va_list expects word-size
sign-extension, so we need to fill the entire word.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 22, 2020
Normal arguments that spill on the stack are packed, but not variadic
arguments. This is handled correctly for their placement already, but
code generated on the callee side with va_list expects word-size
sign-extension, so we need to fill the entire word.

Upstreamed as libffi/libffi#577.

Differential Revision: https://phabricator.services.mozilla.com/D87825
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 23, 2020
Normal arguments that spill on the stack are packed, but not variadic
arguments. This is handled correctly for their placement already, but
code generated on the callee side with va_list expects word-size
sign-extension, so we need to fill the entire word.

Upstreamed as libffi/libffi#577.

Differential Revision: https://phabricator.services.mozilla.com/D87825
@lawrence-danna-apple
Copy link

Can you provide a test demonstrating the incorrect behavior that this fixes?

@glandium
Copy link
Contributor Author

glandium commented Oct 6, 2020

It turns out the fix was only necessary because the callee had va_arg() calls with undefined behavior.

@glandium glandium closed this Oct 6, 2020
@glandium
Copy link
Contributor Author

glandium commented Oct 6, 2020

Come to think of it, this may still be necessary.

The problem happens when the callee does e.g. va_arg(list, int), but the caller passed a smaller type, e.g. short, which is promoted to int per the C standard. The caller may cast back to the original type (short), but it should get a valid int from the promotion, but doesn't because libffi doesn't fill all the bits.

A small test that shows the problem looks like the following:

#include <stdio.h>
#include <ffi.h>

int main() {
	short val = -42;
	char *s = "Hello %x\n";
	ffi_cif cif;
	ffi_type* argtypes[] = { &ffi_type_pointer, &ffi_type_sshort };
	void* values[] = { &s, &val };
	ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 1, 2, &ffi_type_void, argtypes);
	ffi_call(&cif, printf, NULL, values);

	printf(s, val);

	return 0;
}

It could be argued that the caller maybe should do the promotion itself, but it could be argued back that ffi should then return an error when it's passed varargs arguments that are supposed to be promoted.

(Note that technically, to printf a short it should be %hx, in which case there is no difference between the libffi call and the direct call, but the point is exactly for when the callee takes shortcuts with the argument type ; also note that clang doesn't warn about the format string for printf("%x", (short)-42) but does for e.g. "%lx").

@glandium glandium reopened this Oct 6, 2020
ambroff pushed a commit to ambroff/gecko that referenced this pull request Nov 4, 2020
Normal arguments that spill on the stack are packed, but not variadic
arguments. This is handled correctly for their placement already, but
code generated on the callee side with va_list expects word-size
sign-extension, so we need to fill the entire word.

Upstreamed as libffi/libffi#577.

Differential Revision: https://phabricator.services.mozilla.com/D87825
ambroff pushed a commit to ambroff/gecko that referenced this pull request Nov 4, 2020
Normal arguments that spill on the stack are packed, but not variadic
arguments. This is handled correctly for their placement already, but
code generated on the callee side with va_list expects word-size
sign-extension, so we need to fill the entire word.

Upstreamed as libffi/libffi#577.

Differential Revision: https://phabricator.services.mozilla.com/D87825
@fkistner
Copy link

fkistner commented Nov 25, 2020

I am also seeing a problem with the ushort return value in libffi.bhaible/test-call.c -DDGTEST=25 -O2 macOS 11 arm64. Could this be related?

@glandium
Copy link
Contributor Author

That would be something else, since it affects a return value, not a variadic argument.

@fkistner
Copy link

Thought this might've also been related to integer promotion, but seems it's a problem in the test setup. Filed as #598.

@atgreen
Copy link
Member

atgreen commented Jun 27, 2021

libffi was recently changed to reject variable argument types that require promotion. This task is left to the caller.

@atgreen atgreen closed this Jun 27, 2021
@dkocher
Copy link

dkocher commented Jun 28, 2021

What does this mean to JNA which has previously cherry picked this PR?

@atgreen
Copy link
Member

atgreen commented Jun 28, 2021

I don't think the fact that they are carrying this patch will break anything, but the right thing for them to do is to promote these types themselves, just like promote float to double for varargs: java-native-access/jna#463

The Ruby FFI layer also does its own promotion above libffi: https://github.com/ffi/ffi/blob/6d14c0a9107c0d5febb3bf92a60d2581e768fa2f/ext/ffi_c/Variadic.c

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Feb 19, 2023
Release (5.13.0)
Features

    #1454: Add c.s.j.p.win32.Psapi.QueryWorkingSetEx and associated Types - @Crain-32.
    #1459: Add VirtualLock and VirtualUnlock in c.s.j.p.win32.Kernel32 - @matthiasblaesing.
    #1471: Add c.s.j.p.win32.Advapi32Util#isCurrentProcessElevated and associated Types - @dbwiddis.
    #1474: Add c.s.j.p.win32.WbemCli#IWbemClassObject.IWbemQualifierSet, IWbemServices.GetObject, IWbemContext.SetValue and associated methods - @rchateauneu.
    #1482: Add multilingual support of Kernel32Util.formatMessage - @overpathz.
    #1490: Adds support for a custom SymbolProvider in NativeLibrary & Library - @soywiz.
    #1491: Update libffi to v3.4.4 - @matthiasblaesing.
    #1487: Add 'uses' information to OSGI metadata in MANIFEST.MF to improve stability of package resolution - @sratz.

Bug Fixes

    #1452: Fix memory allocation/handling for error message generation in native library code (dispatch.c) - @matthiasblaesing.
    #1460: Fix win32 variant date conversion in DST offest window and with millisecond values - @eranl.
    #1472: Fix incorrect bitmask in c.s.j.Pointer#createConstant(int) - @dbwiddis.
    #1481: Fix NPE in NativeLibrary when unpacking from classpath is disabled - @trespasserw.
    #1489: Fixes typo in OpenGL32Util#wglGetProcAddress, instead of parameter procName the hardcoded value wglEnumGpusNV was used - @soywiz.

Release 5.12.1
Bug Fixes

    #1447: Null-check cleanable in c.s.j.Memory#close - @dbwiddis.

Release 5.12.0
Features

    #1433: Add CFEqual, CFDictionaryRef.ByReference, CFStringRef.ByReference to c.s.j.p.mac.CoreFoundation - @shalupov
    #978: Remove use of finalizers in JNA and improve concurrency for Memory, CallbackReference and NativeLibrary - @matthiasblaesing.
    #1440: Support for LoongArch64 - @Panxuefeng-loongson.
    #1444: Update embedded libffi to 1f14b3fa92d4442a60233e9596ddec428a985e3c and rebuild native libraries - @matthiasblaesing.

Bug Fixes

    #1438: Handle arrays in structures with differing size - @matthiasblaesing.
    #1442: Handle race condition in c.s.j.p.win32.PdhUtil#PdhEnumObjectItems - @dbwiddis.

Important Changes

    Memory#dispose, CallbackReference#dispose and NativeLibrary#dispose were called by the Object#finalize override. These calls were replaced by the use of a cleaner. It is not guaranteed anymore, that dispose is called on subclasses on finalization.

Release 5.11.0
Features

    #1398: Increase c.s.j.p.win32.Sspi#MAX_TOKEN_SIZE on Windows 8/Server 2012 and later - @dbwiddis.
    #1403: Rebuild AIX binaries with libffi 3.4.2 (other architectures were part of 5.10) - @matthiasblaesing.
    #1404: Added Solaris Kstat2 library - @dbwiddis.
    #1416: Add CFDictionaryGetCount to c.s.j.p.mac.CoreFoundation - @shalupov
    #1418: Add CertOpenStore to c.s.j.p.win32.Crypt32 - @shalupov

Bug Fixes

    #1411: Do not throw Win32Exception on success for empty section in Kernel32Util#getPrivateProfileSection - @mkarg.
    #1414: Fix definition of c.s.j.p.unix.X11.XK_Shift_R - @matthiasblaesing.
    #1323. Fix crashes in direct callbacks on mac OS aarch64 - @matthiasblaesing.
    #1422: Load jawt library relative to sun.boot.library.path system on unix OSes - @matthiasblaesing.
    #1427: Rebuild all binaries with fix from #1422 and #1323 - @matthiasblaesing.

Release 5.10.0
Features

    #1377: Add RegLoadAppKey to c.s.j.p.win32.Advapi32 and registryLoadAppKey to c.s.j.p.win32.Advapi32Util - @mfilippov.
    #1093: Add OpenFileMapping to c.s.j.p.win32.Kernel32 - @lmitusinski.
    #1388: Map the arch zarch_64 as reported by SAPJVM8 to s390x - @MBaesken.
    #1381: Update embedded libffi to 3.4.2 - @matthiasblaesing.
    #1393: Update native encoding detection for JEP400 / JDK 18 (file.encoding now defaults to UTF-8) - @matthiasblaesing.

Bug Fixes

    #1378: Handle failure in ffi_closure_alloc - @DaveCTurner.

Release 5.9.0
Features

    #1336: Add HKEY_CURRENT_USER_LOCAL_SETTINGS to c.s.j.p.win32.WinReg - @Dani-Hub.
    #1337: Add REG_NOTIFY_THREAD_AGNOSTIC to c.s.j.p.win32.WinNet and update REG_LEGAL_CHANGE_FILTER - @Dani-Hub.
    #1338: Add RegNotifyChangeKeyValue to c.s.j.p.win32.Advapi32 - @Dani-Hub.
    #1340: Add CM_Get_DevNode_Registry_Property to c.s.j.p.win32.Cfgmgr32 and corresponding util in c.s.j.p.win32.Cfgmgr32Util - @dbwiddis.
    #1352: Add BringWindowToTop to c.s.j.p.win32.User32 - @kahgoh.
    #1354: Add GetParent to c.s.j.p.win32.User32 - @kahgoh.
    #1360: Add CommandLineToArgvW to c.s.j.p.win32.Shell32 and corresponding util in c.s.j.p.win32.Shell32Util - @dbwiddis.
    #1363: Update NUMA_NODE_RELATIONSHIP in c.s.j.p.win32.WinNT to new version of the structure and improve support for future values of c.s.j.p.win32.WinNT.LOGICAL_PROCESSOR_RELATIONSHIP - @dbwiddis.

Bug Fixes

    #1343: c.s.j.p.mac.CoreFoundation.CFStringRef#stringValue buffer needs space for a null byte - @dbwiddis.
    #1351: Define c.s.j.p.unix.size_t.ByReference and fix macOS sysctl size_t * parameters - @dbwiddis.
    #1362: Clear security sensitive data after usage in c.s.j.p.win32.Crypt32Util#cryptProtectData and #cryptUnprotectData - @dmytro-sheyko.
    #1361: Make c.s.j.p.win32.Crypt32Util#cryptProtectData and #cryptUnprotectData properly handle 0-length array as input - @dmytro-sheyko.

Release 5.8.0
Features

    #1313: Normalize RESOURCE_PREFIX for darwin to darwin-$arch and split jnidispatch library per architecture - @matthiasblaesing.
    #1318: Add support for linux-riscv64 - @thentschel.
    #1327: Add partial support for future values of c.s.j.p.win32.WinNT.LOGICAL_PROCESSOR_RELATIONSHIP enum present in Windows Insider builds - @dbwiddis.

Bug Fixes

    #1317: Change the maven coordinates of the JPMS artifacts from classifier jpms to custom artifact ids jna-jpms and jna-platform-jpms - @matthiasblaesing.
    #1322: Handle 0-length domain names in c.s.j.p.win32.Advapi32Util#getAccountBySid - @dbwiddis.
    #1326: Ensure pointers indirected from Memory and pointing into Memory retain originating object - @matthiasblaesing.

Important Changes

    The maven coordinates of the experimental JPMS (java module system) artifacts were moved from using the classifier jpms to custom artifact ids jna-jpms and jna-platform-jpms, without an classifier. The reason for this is, that the platform artifacts depend on the jna artifacts and need to pull in the right variant. This is not possible if the classifier is used.

    RESOURCE_PREFIX for darwin (mac OS) was changed from darwin to darwin-$arch as the fat binaries on mac OS causes various problems: It was reported, that binaries were rejected from the appstore because x86 binaries were found in the application (jnidispatch for mac OS x86) and that builds needed to be special cased so that the native library can be assembled. The latter is also true for JNA.
    While the prefix is changed, the old prefix is still searched as a fallback location, so if only a fat binary is present, it can still be loaded.

Release 5.7.0
Features

    #1301: Improve bindings of the printer notification functions (FindFirstPrinterChangeNotification, FindNextPrinterChangeNotification) in c.s.j.p.w.Winspool - @ianjoneill.
    #1238: Add macOS aarch64 architecture to universal darwin target. Cherry pick libffi/libffi#577. - @fkistner, @Vzor-, @tresf.
    #1264: Update libffi to v3.3; Add Windows aarch64 target. - @tresf.
    #1293: Bind part of Windows Application Recovery and Restart API: RegisterApplicationRestart, UnregisterApplicationRestart and GetApplicationRestartSettings in c.s.j.p.w.Kernel32 - @matthiasblaesing.
    #1217: Add mappings for AIX Perfstat library to c.s.j.p.unix.aix - @dbwiddis.
    #1231: The test suite can now be executed on Windows using either ANSI or UNICODE win32 API by passing -Dw32.ascii=true/false to ant. Previously, UNICODE was always used. - @T-Svensson
    #1237: Experimental: Add artifacts that make jna and jna-platform named modules (provide module-info.class). The new artifacts are named jna-jpms.jar and jna-platform-jpms.jar - @matthiasblaesing.
    #1242: Add CallWindowProc to User32 - @heldplayer.
    #1239: Improve performance of allocation of c.s.j.Memory objects - @joerg1985.
    #1246: Improve performance of c.s.j.Structure#read and c.s.j.Structure#write - @joerg1985.
    #1260: Add mapping for X11 generic events - @lafoletc.
    #1263: Add LowLevelMouseProc - @nordiakt.
    #1265: Add mapping for XQueryExtension - @lafoletc.
    #1299: Add c.s.j.p.win32.IPHlpApi#GetExtendedTcpTable, c.s.j.p.win32.IPHlpApi#GetExtendedUdpTable, and supporting structures. - @dbwiddis.

Bug Fixes

    #1286: Fix bindings of c.s.j.p.win32.DBT - @matthiasblaesing.
    #326: Fix loading library that re-uses pointers for different callbacks - @fpapai.
    #1244: Fix building on GCC 10 - @matthiasblaesing.
    #1252: - Fix bindings of CTL_ENTRY#getRgAttribute, CTL_INFO#getRgCTLEntry, CTL_INFO#getRgExtension, CERT_EXTENSIONS#getRgExtension, CERT_INFO#getRgExtension, CRL_INFO#getRgCRLEntry, CRL_INFO#getRgExtension, CRL_ENTRY#getRgExtension. Add bindings for CertEnumCertificatesInStore, CertEnumCTLsInStore, CertEnumCRLsInStore and CryptQueryObject in c.s.j.p.win32.Crypt32.
    WARNING: The signatures for CTL_INFO#getRgCTLEntry and CTL_INFO#getRgExtension were changed - as the original signatures were obviously wrong and read the wrong attributes, it is not considered an API break - @matthiasblaesing.
    #1275: Fix CFStringRef#stringValue for empty Strings - @dyorgio.
    #1279: Remove DLLCallback import from CallbackReference - @dyorgio.
    #1278: Improve compatibility of c.s.j.p.WindowUtils#getProcessFilePath and fix unittests for windows 32bit intel - @matthiasblaesing.
    #1284: Fix illegal access exceptions, when retrieving options for private library interfaces with an instance field - @fkistner.
    #1300: Deprecate c.s.j.p.win32.WTypes.BSTR String constructor and setValue method, as BSTR allocation should be managed by COM, Automation, and Interop functions - @dbwiddis.

Breaking Changes

    Prebuild native library for darwin x86 (32bit java on mac OS) was removed

Release 5.6.0
Features

    #1160: Make FileUtils#moveToTrash a varargs method - @matthiasblaesing.
    #1167: Add c.s.j.p.win32.Kernel32#GetProcessAffinityMask - @dbwiddis.
    #1168: Add c.s.j.p.win32.Kernel32#SetProcessAffinityMask - @dbwiddis.
    #1169: Wait for process in getLinuxLdPaths - @rdesgroppes.
    #1178: Add c.s.j.p.win32.IPHlpAPI#GetTcpStatistics, c.s.j.p.win32.IPHlpAPI#GetUdpStatistics, c.s.j.p.win32.IPHlpAPI#GetTcpStatisticsEx and c.s.j.p.win32.IPHlpAPI#GetUdpStatisticsEx - @dbwiddis.
    #1182: Add toString to classes extending c.s.j.ptr.ByReference - @dbwiddis.
    #1191: Add c.s.j.p.win32.Advapi32Util#getTokenPrimaryGroup - @dbwiddis.
    #1194: Add GetConsoleScreenBufferInfo, ReadConsoleInput and WriteConsole with associated structures to c.s.j.p.win32.Wincon - @rednoah.
    #1198: Add NetSessionEnum to c.s.j.p.win32.Netapi32 and WTSEnumerateSessions, WTSQuerySessionInformation, and WTSFreeMemory to c.s.j.p.win32.Wtsapi32 - @dbwiddis.
    #1200: Add mappings for libudev to c.s.j.p.linux.Udev - @dbwiddis.
    #1202: Add mappings supporting shared memory including c.s.j.p.unix.LibCAPI types size_t and ssize_t, c.s.j.p.linux.LibC methods munmap(), msync(), and close(), c.s.j.p.unix.LibCUtil mapping mmap() and ftruncate(), and c.s.j.p.linux.LibRT methods shm_open() and shm_unlink() - @dbwiddis.
    #1209: Add mappings for Thread32First and Thread32Next to c.s.j.p.win32.Kernel32 - @dbwiddis.
    #1214: Add mapping for EnumProcesses to c.s.j.p.win32.Psapi and c.s.j.p.win32.PsapiUtil - @T-Svensson.

Bug Fixes

    #1183: c.s.j.p.win32.WinDef.CHARByReference#getValue should only read one byte - @dbwiddis.
    #1184: c.s.j.p.win32.WinDef.ULONGLONG should always be 8 bytes - @dbwiddis.
    #1196: c.s.j.p.win32.WinNT.LARGE_INTEGER needs to populate both union fields - @dbwiddis.
    #1216: Failure loading frameworks on macOS 11 - @dkocher.

Release 5.5.0
Features

    #1131: Add CoreFoundation, IOKit, and DiskArbitration mappings in c.s.j.p.mac. @dbwiddis.
    #1143: c.s.j.p.mac.SystemB now extends c.s.j.p.unix.LibCAPI. @dbwiddis.
    #1147: Add additional OSGi headers for the JNA bundle to support 32bit ARM (hardfloat) @mattixtech.
    #1148/#1096: Include Win32 COM utils (c.s.j.p.win32.com.util and c.s.j.p.win32.com.annotation) in OSGI bundle @dbwiddis.

Bug Fixes

    #1115: Fix signature for c.s.j.p.win32.Kernel32#CreateRemoteThread and bind VirtualAllocEx, VirtualFreeEx, GetExitCodeThread in c.s.j.p.win32.Kernel32 - @apangin, @matthiasblaesing.
    #1127: Windows needs a wide string in c.s.j.p.win32.COM.IShellFolder#ParseDisplayName - @dbwiddis.
    #1128: KEY_ALL_ACCESS value is incorrect in c.s.j.p.win32.WinNT.java - @trevormaggs.
    #1133: Ensure JARs created from the build system don't contain invalid Info-ZIP Unicode Path extra info - @matthiasblaesing.
    #1134: Read correct member of WinBase.SYSTEM_INFO.processorArchitecture union - @dbwiddis.
    #1118: Fix passing unions containing integer and floating point members as parameters by value - @matthiasblaesing.

Release 5.4.0
Features

    #1105: Deprecate c.s.j.p.win32.Advapi32Util.EventLogRecord#getEventId in favor of #getInstanceId - @dbwiddis.
    #1097: Allow .ocx as extension of native libraries on windows - @dmigowski.
    #1108: Improve performance of c.s.j.Structure#newInstance by iteration available constructors instead of exception handling @bjorndarri.

Bug Fixes

    #1095 Align behaviour of com.sun.jna.platform.macXAttrUtil#setXattr and #getXAttr with CLI tool - @jrobhoward, @matthiasblaesing.
    #1091: Check target number to be greater than zero, before calling Structure#toArray in c.s.j.p.win32.Netapi32Util - @trevormagg, @matthiasblaesing.
    #1103: On big endian architecture IntegerType based values are incorrectly decoded when using direct binding - @matthiasblaesing.

Release 5.3.1
Bug Fixes

    #1089: c.s.j.internal.ReflectionUtils accesses java.lang.invoke.MethodType without reflection, causing java.lang.NoClassDefFoundError on android API level < 26 - @matthiasblaesing.
    #1087: Fix wrong calls to Structure#toArray with zero sized arrays - @matthiasblaesing.

Release 5.3.0
Features

    #1058: Add selectable timeout to stopService() and improve timeout handling - @keithharp.
    #1050: Add c.s.j.p.win32.VersionHelpers and supporting functions - @dbwiddis.
    #1061: replace toArray(new T[size]) with toArray(new T[0]) for better performance - @hc-codersatlas.
    #1064: Add c.s.j.p.win32.Kernel32.GetLogicalProcessorInformationEx function, convenience Util method and supporting structures - @dbwiddis.
    #1065: Add c.s.j.p.win32.PowrProf#CallNTPowerInformation and supporting structures - @dbwiddis.
    #1063: Enhance c.s.j.p.win32.User32 and associated classes to support keyboard related functionality. - @kevemueller.
    #1068: c.s.j.p.win32.Advapi32Util.getAccountBySid(String systemName, PSID sid) ignored parameter instead of passing it to the native function - @nirud.
    #813: Support for default methods on interfaces (experimental) - @matthiasblaesing.
    #1073: Support COM setters with multiple parameters using c.s.j.p.win32.COM.util.ProxyObject - @matthiasblaesing.
    #1083: Prevent access to unsupported values in c.s.j.p.win32.COM.WbemcliUtil#enumerateProperties and bind c.s.j.p.win32.COM.Wbemcli.IWbemClassObject.GetNames - @matthiasblaesing.

Bug Fixes

    #1052, #1053: WinXP compatibility for c.s.j.p.win32.PdhUtil - @dbwiddis.
    #1055: Include c.s.j.p.linux in OSGi bundle. - @dbwiddis.
    #1066: On AIX OpenJDK differs from IBM J9 in the mapping of library names. While J9 maps jnidispatch to libjnidispatch.a, OpenJDK maps to libjnidispatch.so, which causes the native library extractor to fail. AIX is now hard-coded to libjnidispatch.a - @matthiasblaesing.
    #1079: Fix maximum structure alignment for Android i386 - @BugsBeGone.
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 this pull request may close these issues.

None yet

5 participants