[10] RFR(S) 8183262: noexecstack check in os::dll_load on Linux is too expensive

[Vladimir Kozlov]

Thank you Thomas

On 7/4/17 10:54 PM, Thomas Stüfe wrote:
> Hi Vladimir,
> 
> thanks for taking my suggestion. This looks ok.
> 
> The mixup of return values for file errors (true for fopen-failed?) is
> weird, but that was weird before the patch. You could use access(3) to

Yes, I also was baffled by it. That is why I copied original comment 
from .hpp file into the code. It lists cases when this method returns 
true or false.

> determine if the shared object can be accessed before using
> specifies_noexecstack(). But I leave this up to you, the fix is fine for me
> as it is. No need for a new webrev.

I would like to keep fix as it is. I want to avoid submitting an other 
RBT testing which is unstable.

Thanks,
Vladimir

> 
> Thanks, Thomas
> 
> 
> On Tue, Jul 4, 2017 at 9:13 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com>
> wrote:
> 
>> Here is updated changes which converted ElfFile::specifies_noexecstack()
>> to static method.
>>
>> http://cr.openjdk.java.net/~kvn/8183262/webrev.01/
>>
>> RBT testing passed.
>>
>> AOT initialization time is still small - new code reads only first block
>> from file.
>>
>> Thanks,
>> Vladimir
>>
>>
>> On 6/30/17 3:52 PM, Vladimir Kozlov wrote:
>>
>>> Thank you, Thomas
>>>
>>> I thought about that but choose to do this since I will have to duplicate
>>> checks done in constructor.
>>>
>>> But starting looking on it I realized that my fix is incorrect because
>>> load_tables() reads file header which is used by specifies_noexecstack().
>>> It is mess - os::dll_load() also reads header later.
>>>
>>> Anyway, to make my life easy I will duplicate header read code in
>>> specifies_noexecstack().
>>>
>>> I will send updated webrev after I test it.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 6/30/17 12:44 PM, Thomas Stüfe wrote:
>>>
>>>> Hi Vladimir,
>>>>
>>>> This seems fine, but I am a bit concerned that this misuses an
>>>> inconsistency in the API, namely the fact that specifies_noexecstack()
>>>> behaves differently than all other functions (re-read the file each time
>>>> it
>>>> is called instead of using cached information). In order to understand
>>>> the
>>>> "load_tables" argument in the constructor, one has to understand that odd
>>>> design, and know that load_tables=false means the only method which can
>>>> be
>>>> used is specifies_noexecstack().
>>>>
>>>> As a proposal, could you instead make specifies_noexecstack() a static
>>>> function in ElfFile, such that:
>>>>
>>>> static bool ElfFile::specifies_noexecstack(const char* filename);
>>>>
>>>> Then it would be immediately clear that this function is to be used
>>>> without
>>>> constructing the ElfFile object.
>>>>
>>>> Kind Regards, Thomas
>>>>
>>>> On Fri, Jun 30, 2017 at 7:07 PM, Vladimir Kozlov <
>>>> vladimir.kozlov at oracle.com
>>>>
>>>>> wrote:
>>>>>
>>>>
>>>> webrev: http://cr.openjdk.java.net/~kvn/8183262/webrev/
>>>>> https://bugs.openjdk.java.net/browse/JDK-8183262
>>>>>
>>>>> To load AOT library we use os::dll_load() method provided on all
>>>>> platforms.
>>>>>
>>>>> During investigation why AOT library load takes so long on Linux I found
>>>>> that it spend most of time in ElfFile::load_tables() method which looks
>>>>> for
>>>>> string tables in library. It is called from ElfFile() constructor used
>>>>> in
>>>>> os::dll_load() just to check if library is built with noexecstack:
>>>>>
>>>>>    if (os::uses_stack_guard_pages() && !os::Linux::_stack_is_executable)
>>>>> {
>>>>>      ElfFile ef(filename);
>>>>>      if (!ef.specifies_noexecstack()) {
>>>>>
>>>>> There is no need for string tables load in such case. Avoiding loads
>>>>> helps
>>>>> a lot:
>>>>>
>>>>> without fix:
>>>>>
>>>>> [0.047s][info][aot,startuptime] AOT initialization, 0.0424919 secs
>>>>>
>>>>> with fix:
>>>>>
>>>>> [0.002s][info][aot,startuptime] AOT initialization, 0.0002087 secs
>>>>>
>>>>>
>>>>> Note, JDK 9 is not affected because AOT uses system dlopen() since only
>>>>> Linux is supported.
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>>