[PATCH] JDK-8214122 Prevent name mangling of jdwpTransport_OnLoad in Windows 32-bit DLL name decoration (original) (raw)

Ali İnce ali.ince at gmail.com
Mon Dec 10 21:13:57 UTC 2018

Hi Alexey,

I’ve searched for |GetProcAddress| usages across source code and couldn’t find (hopefully tbh) other occurrences of such mismatches.

Regards,

Ali

On 7 Dec 2018, at 20:24, Alexey Ivanov <alexey.ivanov at oracle.com> wrote:

Hi Ali, On 06/12/2018 22:49, Ali İnce wrote: Hi Magnus, Alexey,

I believe we won’t be able to get further opinions from serviceability-dev. Unfortunately, no one has replied as of now. Have you found any issues with jdwpTransportOnLoad after removing JNICALL? Andrew Luo suggested using a similar mechanism as is used for jvm.dll by using symbol name files mapped by platform (files are under make/hotspot/symbols and interestingly windows is almost the only platform for which a symbol file doesn’t exist). Andrew says the .def files are auto-generated for Windows. There's a set of shared functions. JVM cannot change the calling convention, jvm.dll is the public interface to JVM. Another issue, again opened against AdoptOpenJDK 32-bit builds is somehow related with the same problem - but this time it is jimage.dll depending on zip.dll expecting the ZIPInflateFully function to be decorated with JNICALL - whereas it was removed earlier from the implementation and the runtime image just fails with access violation just because jimage source code still declared it with JNICALL (https://github.com/AdoptOpenJDK/openjdk-build/issues/763 <https://github.com/AdoptOpenJDK/openjdk-build/issues/763>). The usage of ZIPInflateFully from zip.dll by jimage.dll was overlooked during work on https://bugs.openjdk.java.net/browse/JDK-8201226 <https://bugs.openjdk.java.net/browse/JDK-8201226>. I can take care of it. (I could not build 32 bit Windows. It seem to have overcome the problem by adding --disable-warnings-as-errors option to configure.) However, the report says the resulting image crashes in 64 bit windows too which shouldn't be affected by JNICALL mismatch. I’ve also searched for GetProcAddress calls across the source code - and I think it’s important to work on the consistency of JNICALL usages across the whole source code. There are many places where libraries are loaded dynamically or a function may be unavailable on older versions of the platform. Have you identified any other place where functions from JDK DLLs are looked up by names? Another noteworthy thing I’ve noticed is there are still JNICALL modifiers for example in https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/jdk.crypto.cryptoki/windows/native/libj2pkcs11/j2secmodmd.c#L49 <https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/jdk.crypto.cryptoki/windows/native/libj2pkcs11/j2secmodmd.c#L49> - which I guess will also be reported as broken since these functions are again name decorated. This is a JNI method. It should use JNICALL modifier. If it wasn't found, the class sun.security.pkcs11.Secmod would fail to load. I guess JVM handles name mangling somehow for native method implementation. Such functions were never explicitly exported using mapfiles or /export options on Windows, at least in the client area. For Linux and Solaris, adding or removing a native method required editing mapfiles. _If we’re still determined to remove JNICALL, what about just removing stdcall from JNICALL declaration at https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/java.base/windows/native/include/jnimd.h#L31 <https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/java.base/windows/native/include/jnimd.h#L31> ? Wouldn’t that be a more consistent move? We can't do that. _Implementation of Java native methods must use stdcall calling convention.

Regards, Ali On 22 Nov 2018, at 17:29, Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com <mailto:magnus.ihse.bursie at oracle.com>> wrote:

I think we are in complete agreement. What's missing is some expert opinion from serviceability-dev if there is any problem with changing the signature. I'd preferably have that. If no one knows, I'd say, change it, and see if we still pass the relevant tests, and if so, ship it. /Magnus 22 nov. 2018 kl. 18:04 skrev Alexey Ivanov <alexey.ivanov at oracle.com <mailto:alexey.ivanov at oracle.com>>:

On 21/11/2018 12:16, Magnus Ihse Bursie wrote: On 2018-11-20 16:49, Alexey Ivanov wrote: Hi Ali, Magnus,

I absolutely agree this change has to be reviewed by someone from serviceability. There are several options: 1. Add -export:jdwpTransportOnLoad to LDFLAGS for Windows http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023935.html <http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023935.html> so it partially reverts the changes from https://bugs.openjdk.java.net/browse/JDK-8200178 <https://bugs.openjdk.java.net/browse/JDK-8200178> _2. Remove JNICALL (_stdcall) modifier, eventually changing the calling convention to cdecl. http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023969.html <http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023969.html> 3. Use inline /export option via #pragma: #pragma comment(linker, "/export:jdwpTransportOnLoad= jdwpTransportOnLoad at 16") as referenced in https://docs.microsoft.com/en-ie/cpp/cpp/dllexport-dllimport?view=vs-2017 <https://docs.microsoft.com/en-ie/cpp/cpp/dllexport-dllimport?view=vs-2017> https://docs.microsoft.com/en-ie/cpp/build/reference/exports?view=vs-2017 <https://docs.microsoft.com/en-ie/cpp/build/reference/exports?view=vs-2017> The third options still needs to be tested to make sure it does not break other platforms builds. I'm not fond of either solution 1 or 3. I really think the signature should be made correctly at the point of definition in the source code. _That is why I proposed removing JNICALL (stdcall) from the function declaration as we had done for libzip, libjimage [1] and mlibimage [2]. _Microsoft recommends using stdcall for DLLs, at the same time it decorates the function name making it harder to export it by its plain name. _By removing JNICALL / _stdcall, we make the function use _cdecl calling convention. The difference between them is that with _stdcall the callee cleans the stack whereas with cdecl the caller cleans the stack. It shouldn't be a problem as long as all function declarations use the same calling convention, which, I believe, is the case here. We've spent some considerable effort of getting rid of the /export flags for Windows (and map files for unix), and I don't want to see them creep back in. Note that option 3 is just option 1 in disguise. The only single good thing about it is that it allows you to put the compiler option next to the signature declaration in the source code. At least in this case, the option will be near the function body definition. It will be easier to update it if the signature changes. _If we are to use stdcall, it seems to be the only way to export the undecorated name. The same goes for def files, as suggested by Ali. It's just yet another version of option 1 in disguise/map files. If option 2 is rejected, I'm for using option 3. If option 3 cannot be used too, I'm for option 1. I think we should not fall back to def files in any case. And Makefile will have to include the reference to the def file, so it's even worse than option 1. Regards, Alexey [1] https://bugs.openjdk.java.net/browse/JDK-8201226 <https://bugs.openjdk.java.net/browse/JDK-8201226> [2] https://bugs.openjdk.java.net/browse/JDK-8202476 <https://bugs.openjdk.java.net/browse/JDK-8202476> /Magnus

Regards, Alexey On 19/11/2018 12:19, Magnus Ihse Bursie wrote: Hi Ali, From a build perspective this change looks OK. I'm not aware of the finer details on the OnLoad mechanism, like what calling convention is to be used. So maybe this is a no-go from that view. I'm cc:ing servicability so they can have a look at it. /Magnus On 2018-11-18 13:07, Ali İnce wrote: Hi Alexey, _Below is a new patch content that removes JNICALL modifiers from the exported functions of interest. This results in exported functions not to be name decorated and use cdecl calling convention. Do you have any more suggestions, or would you please guide me on next steps? Thanks, Ali # HG changeset patch # User Ali Ince <ali.ince at gmail.com> <mailto:ali.ince at gmail.com> # Date 1542542415 0 # Sun Nov 18 12:00:15 2018 +0000 # Node ID a41df44e13e1b761f4b3f05a17ed18953067ae39 # Parent 16d5ec7dbbb49ef3f969e34be870e3f917ea89ff Remove JNICALL modifiers to prevent name decoration on 32-bit windows builds diff -r 16d5ec7dbbb4 -r a41df44e13e1 src/jdk.jdi/share/native/libdtshmem/shmemBack.c --- a/src/jdk.jdi/share/native/libdtshmem/shmemBack.c Tue Aug 14 14:28:23 2018 +0200 +++ b/src/jdk.jdi/share/native/libdtshmem/shmemBack.c Sun Nov 18 12:00:15 2018 +0000 @@ -339,7 +339,7 @@ return JDWPTRANSPORTERRORNONE; } -JNIEXPORT jint JNICALL +JNIEXPORT jint jdwpTransportOnLoad(JavaVM vm, jdwpTransportCallback cbTablePtr, jint version, jdwpTransportEnv** result) { diff -r 16d5ec7dbbb4 -r a41df44e13e1 src/jdk.jdwp.agent/share/native/include/jdwpTransport.h --- a/src/jdk.jdwp.agent/share/native/include/jdwpTransport.h Tue Aug 14 14:28:23 2018 +0200 +++ b/src/jdk.jdwp.agent/share/native/include/jdwpTransport.h Sun Nov 18 12:00:15 2018 +0000 @@ -140,7 +140,7 @@ void (*free)(void buffer); / Call this for all deallocations */ } jdwpTransportCallback; -typedef jint (JNICALL *jdwpTransportOnLoadt)(JavaVM *jvm, +typedef jint (*jdwpTransportOnLoadt)(JavaVM *jvm, jdwpTransportCallback *callback, jint version, jdwpTransportEnv** env); diff -r 16d5ec7dbbb4 -r a41df44e13e1 src/jdk.jdwp.agent/share/native/libdtsocket/socketTransport.c --- a/src/jdk.jdwp.agent/share/native/libdtsocket/socketTransport.c Tue Aug 14 14:28:23 2018 +0200 +++ b/src/jdk.jdwp.agent/share/native/libdtsocket/socketTransport.c Sun Nov 18 12:00:15 2018 +0000 @@ -1019,7 +1019,7 @@ return JDWPTRANSPORTERRORNONE; } -JNIEXPORT jint JNICALL +JNIEXPORT jint jdwpTransportOnLoad(JavaVM vm, jdwpTransportCallback cbTablePtr, jint version, jdwpTransportEnv** env) {

On 16 Nov 2018, at 23:03, Alexey Ivanov <alexey.ivanov at oracle.com> <mailto:alexey.ivanov at oracle.com> wrote: Hi Ali, It's exactly what I referred to. Yes, it does change the calling convention. Yet it's not a (big) problem because both parties will use the same calling convention. Using a DLL from another build will not be possible. But it's not a good idea to do so anyway. Mapfiles and export options have been removed by https://bugs.openjdk.java.net/browse/JDK-8200178 <https://bugs.openjdk.java.net/browse/JDK-8200178> to simplify managing exported functions. So that to add or remove an exported function, you don't have modify makefiles and/or mapfiles. You just edit the source files. Regards, Alexey On 16/11/2018 22:42, Ali İnce wrote: Hi Alexey, Thanks for your reply. _I will definitely give it a try though I’m not sure if that’ll be a breaking change. This is because JNICALL modifier is defined to be stdcall on windows which is both specified on jdwpTransportOnLoad exported functions both for dtsocket.dll and dtshmem.dll. _The stdcall is ignored on x64 platforms and also name decoration is not applied (https://docs.microsoft.com/en-gb/cpp/build/reference/decorated-names?view=vs-2017 <https://docs.microsoft.com/en-gb/cpp/build/reference/decorated-names?view=vs-2017>). I believe that’s why we don’t experience any problems on x64 builds. _Removing JNICALL (thus stdcall) modifier (apparently only applies to x86 builds) will result in the calling convention to be changed, and thus will change how parameters are ordered and also the stack cleanup rules. I think this may result in problems in programs that are designed to load shared libraries and its exported functions at runtime (not bound by link time which probably won’t cause any issues - assuming that they use the shared function signature) - as in https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/5f01925b80ed851b133ee26fbcb07026ac04149e/src/jdk.jdwp.agent/share/native/libjdwp/transport.c#L99 <https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/5f01925b80ed851b133ee26fbcb07026ac04149e/src/jdk.jdwp.agent/share/native/libjdwp/transport.c#L99>. Any thoughts? Regards, Ali On 15 Nov 2018, at 22:14, Alexey Ivanov <alexey.ivanov at oracle.com> <mailto:alexey.ivanov at oracle.com> wrote: Hi Ali, Can the issue be resolved by using different call modifiers on the affected functions? Some build / link issues were resolved by adding / removing JNICALL modifier, see https://bugs.openjdk.java.net/browse/JDK-8201226 <https://bugs.openjdk.java.net/browse/JDK-8201226> http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021553.html <http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021553.html> https://bugs.openjdk.java.net/browse/JDK-8202476 <https://bugs.openjdk.java.net/browse/JDK-8202476> http://mail.openjdk.java.net/pipermail/build-dev/2018-May/021913.html <http://mail.openjdk.java.net/pipermail/build-dev/2018-May/021913.html> Regards, Alexey On 15/11/2018 21:43, Ali İnce wrote: Hi, An issue (https://github.com/AdoptOpenJDK/openjdk-build/issues/709 <https://github.com/AdoptOpenJDK/openjdk-build/issues/709>) raised against AdoptOpenJDK jdk11 32-bit windows builds led us to the name decoration problem on built 32-bit dlls - specifically dtsocket.dll and dtshmem.dll. Whereas 64-bit MSVC builds don’t apply any name decorations on exported functions, 32-bit builds apply them - which requires either def files or /export flags to be specified on the linker arguments. Although the expected linker arguments were present on previous jdk makefiles, they were removed in jdk11 makefiles. Below is the proposed patch, which can also be browsed at https://github.com/AdoptOpenJDK/openjdk-jdk11u/pull/1 <https://github.com/AdoptOpenJDK/openjdk-jdk11u/pull/1>. Would you please have a look and let me know of any points I may have missed (I don’t have any background information about why the export flags were removed in jdk11)? Regards, Ali diff --git a/make/lib/Lib-jdk.jdi.gmk b/make/lib/Lib-jdk.jdi.gmk index 197b95c2e2..ac6ebf5591 100644 --- a/make/lib/Lib-jdk.jdi.gmk +++ b/make/lib/Lib-jdk.jdi.gmk @@ -37,6 +37,7 @@ ifeq ($(OPENJDKTARGETOS), windows) _jdk.jdwp.agent:include _ _jdk.jdwp.agent:libjdwp/export, _ _LDFLAGS := $(LDFLAGSJDKLIB), _ _+ LDFLAGSwindows := -export:jdwpTransportOnLoad, _ _LIBS := $(JDKLIBLIBS), _ )) diff --git a/make/lib/Lib-jdk.jdwp.agent.gmk b/make/lib/Lib-jdk.jdwp.agent.gmk index 0bc93e0d35..0382e55325 100644 --- a/make/lib/Lib-jdk.jdwp.agent.gmk +++ b/make/lib/Lib-jdk.jdwp.agent.gmk _@@ -37,6 +37,7 @@ (eval(eval (eval(call SetupJdkLibrary, BUILDLIBDTSOCKET, _ _libjdwp/export, _ _LDFLAGS := $(LDFLAGSJDKLIB) _ _$(call SETSHAREDLIBRARYORIGIN), _ _+ LDFLAGSwindows := -export:jdwpTransportOnLoad, _ _LIBSlinux := -lpthread, _ _LIBSsolaris := -lnsl -lsocket, _ _LIBSwindows := $(JDKLIBLIBS) ws232.lib, _

More information about the build-dev mailing list