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

Thu Feb 27 14:59:13 UTC 2020

On 2020-02-27 15:52, Bob Vandette wrote:
>
>> On Feb 27, 2020, at 7:15 AM, Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com> wrote:
>>
>> 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.
> Thanks, I’ll update them.
>
>> 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?
> I did verify that the quoting works on Mac and Linux.  I needed to add \” before hidden or the quotes would be eliminated causing
> the compiler to complain that visibility was expecting a string but didn’t see one.
Very good. Just goes to show how much all these years in the build team 
has helped me learn how to spot quoting errors without trying (viz., not 
much :)).

Looks good to me, then. (I can't say if this fix still needs a CSR, though.)

/Magnus

>
> Bob.
>
>
>> /Magnus
>>
>>> Bob.
>>>
>>>
>>>> On Feb 26, 2020, at 10:35 AM, Magnus Ihse Bursie <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> 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.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>