> On Feb 27, 2020, at 9:59 AM, Magnus Ihse Bursie
> <magnus.ihse.bur...@oracle.com> wrote:
>
> On 2020-02-27 15:52, Bob Vandette wrote:
>>
>>> On Feb 27, 2020, at 7:15 AM, Magnus Ihse Bursie
>>> <magnus.ihse.bur...@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.)
I’m going to proceed with the CSR since I’m altering the behavior of JNIEXPORT
in jni.h, which is a public header file.
Bob.
>
> /Magnus
>
>>
>> Bob.
>>
>>
>>
>>> /Magnus
>>>
>>>
>>>> Bob.
>>>>
>>>>
>>>>
>>>>> On Feb 26, 2020, at 10:35 AM, Magnus Ihse Bursie
>>>>> <magnus.ihse.bur...@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.bur...@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.bur...@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.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>
