RFR: 8239563 - Reduce public exports in dynamic libraries built from static JDK libraries

Wed Feb 26 15:35:14 UTC 2020

On 2020-02-26 15:56, Bob Vandette wrote:
>
>> On Feb 26, 2020, at 9:17 AM, Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com> wrote:
>>
>> On 2020-02-26 14:31, Bob Vandette wrote:
>>>> On Feb 26, 2020, at 7:31 AM, Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com> wrote:
>>>>
>>>> On 2020-02-26 08:41, David Holmes wrote:
>>>>> Hi Bob,
>>>>>
>>>>> Adding build-dev.
>>>> Thanks for noticing that we were missing, David!
>>> Sorry, I should have included you folks.
>>>
>>>>> On 26/02/2020 6:37 am, Bob Vandette wrote:
>>>>>> Please review this RFE that alters the visibility of JNI entrypoints to hidden when a shared library
>>>>>> is created using static JDK libraries.
>>>>>>
>>>>>> RFE:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8239563
>>>>>>
>>>>>> WEBREV:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~bobv/8239563/webrev.00/
>>>>>>
>>>>>> CSR:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8239791
>>>>>>
>>>>>> All JNI entrypoints that exist in JDK static libraries are declared as exported or visible.
>>>>>> If a dynamic library is built from these static libraries, we end up with many exported
>>>>>> functions that we don't want to provide access to,
>>>>>>
>>>>>> This RFE will change the definition of JNIEXPORT for libraries built when JNI_STATIC_BUILD
>>>>>> is defined.  When defined, functions declared with JNIEXPORT will not be exported when
>>>>>> linked into shared or dynamic libraries.  This will still allow linking of these functions into
>>>>>> dynamic libraries but will not export the JDK symbols outside of the shared library.
>>>>>>
>>>>>> A CSR has been filed (https://bugs.openjdk.java.net/browse/JDK-8239791) to add the JNI_STATIC_BUILD
>>>>>> define support in jni.h.
>>>>> I have reservations about turning this into something we have to expose and support, rather than being something totally handled within the OpenJDK build system.
>>>> I fully agree. The JNI headers are an exported interface. Our internal build mechanisms have nothing to do there.
>>>>
>>>>> I'm tempted to suggest the header files be adjusted to have:
>>>>>
>>>>>      #ifndef JNIEXPORT
>>>>>      <JNIEXPORT basic definitions as they are now >
>>>>>      #endif
>>>>>
>>>>> and then we define the modified JNIEXPORT via the build system when doing a static build.
>>>>>
>>>>> Is that feasible?
>>>> It's definitely doable, and a far better solution.
>>> Yes, I like this solution.
>>>
>>>> A patch something akin to this would be needed:
>>>>
>>>> diff --git a/make/autoconf/flags-cflags.m4 b/make/autoconf/flags-cflags.m4
>>>> --- a/make/autoconf/flags-cflags.m4
>>>> +++ b/make/autoconf/flags-cflags.m4
>>>> @@ -709,7 +709,10 @@
>>>>     # JDK libraries.
>>>>     STATIC_LIBS_CFLAGS="-DSTATIC_BUILD=1"
>>>>     if test "x$TOOLCHAIN_TYPE" = xgcc || test "x$TOOLCHAIN_TYPE" = xclang; then
>>>> -    STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections -fdata-sections"
>>>> +    STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections \
>>>> +        -fdata-sections -DJNIEXPORT=__attribute__((visibility(\"hidden\")))"
>>>> +  else
>>>> +    STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -DJNIEXPORT="
>>>>     fi
>>>>     if test "x$TOOLCHAIN_TYPE" = xgcc; then
>>>>       # Disable relax-relocation to enable compatibility with older linkers
>>>>
>>>> (With the reservation that I just wrote this on the fly and have not tested it -- things like quoting might be off. Also, I'm not sure if the match of
>>>> compilers is correct -- it might be the case that all compilers except Microsoft defines __GNUC__, so maybe the addition of this -D flag might need
>>>> a separate if statement to cover all our compilers correctly.)
>>> Most of the STATIC_BUILD support is done in jni_util.h.  We could define JNIEXPORT in that header file after allowing it to be overridden in jni.h.
>> I'm not sure I understand you correctly here. Do you mean that you'd like to re-define JNIEXPORT inside jni_util.h instead of using compiler command line flags? I don't think that'd work -- all libraries using JNIEXPORT that does not include jni_util.h first would then export their symbols just the same. Even if you fixed those, the system would be very fragile.
> I was just trying to keep all static library building options in one place.  The static libraries that we produce need to include jni_util.h
> or the JNI_OnLoad_xxx functions will not be declared properly.  Why not expand that dependency to the JNIEXPORT?
Unless *all* libraries that include jni.h also include jni_util.h, then 
the current definition of JNIEXPORT in jni.h will be used -- meaning 
that the so decorated functions will be exported -- which was exactly 
what you wanted to prevent. So I fail to see how this can be a solution.
>
> Do we really have access to all of these compiler defines from within our Makefiles?
>
> #if (defined(__GNUC__) && ((__GNUC__ > 4) || (__GNUC__ == 4) && (__GNUC_MINOR__ > 2))) || __has_attribute(visibility)
Well, yes and no. I'm not certain which compilers define __GNUC__ just 
to show compatibility with gcc, but otoh that does not really matter. 
All that matters is that we know how we want JNIEXPORT to be defined 
when creating a static build -- and that we know, since we can check 
which toolchain we're using. (This is btw a far better check than to 
look for __GNUC__).

/Magnus
>
>
> Bob.
>
>
>> /Magnus
>>> Bob.
>>>
>>>> /Magnus
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> BACKGROUND:
>>>>>>
>>>>>> In JDK8 the JNI specification and JDK implementation was enhanced to support static JNI libraries
>>>>>> but we didn’t consider the issue of exportibility of JNI entrypoint symbols.
>>>>>>
>>>>>>     https://bugs.openjdk.java.net/browse/JDK-8005716
>>>>>>
>>>>>> If developers use these static JDK libraries in order to produce a custom shared library, all of the
>>>>>> JNIEXPORTS will be exposed by this library even if the developer did not choose to export these.
>>>>>> This is a security issue and a potential problem if this library is mixed with other libraries containing
>>>>>> these symbols.
>>>>>>
>>>>>> Bob.
>>>>>>
>>>>>>
>>>>>>