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

Thu Feb 27 12:15:00 UTC 2020

On 2020-02-26 22:01, Bob Vandette wrote:
> Here’s an updated webrev that implementes the suggestion that allows 
> JNIEXPORT in jni.h to be overridden
> and the build limits visibility for static libraries.
>
> If this webrev is accepted, I’ll update the CSR solution to match this 
> implementation.
>
> http://cr.openjdk.java.net/~bobv/8239563/webrev.01
This looks basically ok, but some remarks:

You have forgotten to update the copyright year in the header files.

Also, the quoting looks suspicious. I would have guessed, thinking more 
carefully about this than the post yesterday, that the proper syntax 
would be -DJNIEXPORT='__attribute__((visibility("hidden")))' since 
otherwise the ' will make the \ literal. But, I usually end up being 
wrong about 50% of the time regarding this kind of stuff. :-) Have you 
verified that you get the proper define?

/Magnus

>
> Bob.
>
>
>> On Feb 26, 2020, at 10:35 AM, Magnus Ihse Bursie 
>> <magnus.ihse.bursie at oracle.com 
>> <mailto:magnus.ihse.bursie at oracle.com>> wrote:
>>
>> 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 
>>>> <mailto: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 
>>>>>> <mailto: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.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>
>