RFR: 8130017: use _FORTIFY_SOURCE in gcc fastdebug builds - was : RE: gcc FORTIFY_SOURCE application security flags

[Erik Joelsson]

Hello Matthias,

On 2019-05-08 06:27, Baesken, Matthias wrote:
> Hello,    I looked a bit  more  into it .
> It seems to me ,  that   when  -ffp-contract=off   is available  which is the case with  current gcc versions ,  we want to optimize the 2 special files ( sharedRuntimeTrig.cpp / sharedRuntimeTrans.cpp ).
>   see the following comments :
>
> jdk/make/hotspot/lib/JvmOverrideFiles.gmk
>
> 47# If the FDLIBM_CFLAGS variable is non-empty we know
> 48# that the fdlibm-fork in hotspot can get optimized
> 49# by using -ffp-contract=off on GCC/Clang platforms.
>    ......
> 58  BUILD_LIBJVM_sharedRuntimeTrig.cpp_CXXFLAGS := -DNO_PCH $(FDLIBM_CFLAGS) $(LIBJVM_FDLIBM_COPY_OPT_FLAG)
> 59  BUILD_LIBJVM_sharedRuntimeTrans.cpp_CXXFLAGS := -DNO_PCH $(FDLIBM_CFLAGS) $(LIBJVM_FDLIBM_COPY_OPT_FLAG)
> 60
Will this not just resolve itself if you also add -D_FORTIFY_SOURCE=0 to 
C_O_FLAG_NONE (and all the other optimization flag variables that have 
the value "-O0")?
> But still, setting both -O3 and -O2  in one compile call looks not nice to me .
This may not look nice, but is how we have to do things. The last flag 
on a compiler command line takes precedence. We rely on this to override 
general flags with more specific ones (typically a general flag for the 
whole library with specific ones for certain compilation units). This 
technique is quite common.
>
> In case of  ancient gcc  ***without***  -ffp-contract=off   ,  we might still run into issues for these  2 special files  when _FORTIFY_SOURCE  is set .
> Don't know if this is  still relevant .
> In case  we want to be on the very safe side , we might  need  to  filter out   -D_FORTIFY_SOURCE=2    for these 2 compilation units .

We don't want to filter out flags. It creates very brittle code that is 
likely to break in the future.

For the patch, I think it would make sense to introduce a variable for 
the value -D_FORTIFY_SOURCE=2 (and 
-D_FORTIFY_SOURCE=0/-U_FORTIFY_SOURCE) to avoid repeating it.

/Erik

> Best regards, Matthias
>
>
>
>> Hi David,   thanks for the comment .
>>
>> Currently  I do not see the issue in our fastdebug  builds .
>> So I think the  opt-flag filtering got changed/removed in the years  after the
>> issues were reported .
>>
>>
>> https://bugs.openjdk.java.net/browse/JDK-8047952
>>
>> mentions special O-level settings  for  sharedRuntimeTrig.cpp and
>> sharedRuntimeTrans.cpp .
>>
>> But the files  have optimization set in both  fastdebug and  opt  builds  :
>>
>>   Linux  x86_64  gcc-7 based builds :
>>
>>     fastdebug build  (with the added  -D_FORTIFY_SOURCE=2  flag) :
>>
>> -Werror -O3 -D_FORTIFY_SOURCE=2 -DNO_PCH -ffp-contract=off -O2 -
>> D_FORTIFY_SOURCE=2
>>
>>
>> Opt build  (without  -D_FORTIFY_SOURCE=... ) :
>>
>> -O3 -DNO_PCH -ffp-contract=off -O2  ....
>>
>>
>> (btw.  the setting  of  both    -O3 AND -O2   looks strange to me , but that’s
>> unrelated to my change ;  I  noticed that already in OpenJDK 11  ).
>>
>>
>> Best regards, Matthias
>>
>>
>>
>>
>>> Hi Matthias,
>>>
>>> On 8/05/2019 6:05 pm, Baesken, Matthias wrote:
>>>> Hello, here is  a   webrev,  I used the existing bug
>>>> "JDK-8130017 : use _FORTIFY_SOURCE in gcc fastdebug builds"
>>>>
>>>> Hope that’s fine .
>>> That is fine, but please add a comment to the bug explaining exactly how
>>> you fixed the issue and how the issues raised in the bug description
>>> regarding optimisation levels have been addressed.
>>>
>>> Not a review - I'll leave that to build team. The proof of this will be
>>> in the building and testing.
>>>
>>> Thanks,
>>> David
>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8130017
>>>>
>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8130017.0/
>>>>
>>>>
>>>> Our internal OpenJDK Linux   (x86_64, ppc64, ppc64le , s390x)   fastdebug
>>> builds  are fine with the added flag .
>>>>