RFR(S) 8226406: JVM fails to detect mismatched or corrupt CDS archive
Calvin Cheung
calvin.cheung at oracle.com
Wed Jul 10 07:13:33 UTC 2019
On 7/9/19 5:25 PM, Jiangli Zhou wrote:
> Hi Calvin,
>
> On Mon, Jul 8, 2019 at 11:31 PM Calvin Cheung <calvin.cheung at oracle.com> wrote:
>>
>> On 7/8/19 4:38 PM, Jiangli Zhou wrote:
>>> Hi Calvin,
>>>
>>> - src/hotspot/share/include/cds.h
>>>
>>> 36 #define NUM_CDS_REGIONS 8
>>>
>>> The above change would need to be hand fixed when backporting to older
>>> versions. It's fine to include it in the current review, but it's
>>> better to create a separate bug and commit using that bug ID. So it
>>> will make the backports cleaner.
>> I don't think it is worthwhile filing a bug just for this line.
>>
>> I've added a comment as follows:
>>
>> 36 #define NUM_CDS_REGIONS 8 // this must be the same as
>> MetaspaceShared::n_regions
> The issue is that one needs to manually change or revert the above
> when backporting the change to older JDK versions, so the backport is
> not clean and introduces risks. Committing it under a separate bug id
> will avoid that issue. Or, you could combine the above it with some
> other change later. In general, it's a good practice to avoid
> combining unrelated changes in one changeset (with single bug id).
I've reverted the changes in cds.h and filemap.hpp and filed the
following bug for the NUM_CDS_REGIONS adjustment:
https://bugs.openjdk.java.net/browse/JDK-8227496
Updated webrev:
http://cr.openjdk.java.net/~ccheung/8226406/webrev.02/
thanks,
Calvin
>
>>> --------
>>>
>>> 39 #define CDS_END_MAGIC 0xf00babae
>>>
>>> What's the significance of the new end magic? Should the existing
>>> header validation be sufficient as long as it's done first?
>> It seems unnecessary now. I got rid of it.
>>> --------
>>>
>>> - src/hotspot/share/memory/filemap.cpp
>>>
>>> 901 if (_header->_magic != CDS_ARCHIVE_MAGIC && _header->_magic !=
>>> CDS_DYNAMIC_ARCHIVE_MAGIC) {
>>> 902 unsigned int expected_magic = is_static ? CDS_ARCHIVE_MAGIC :
>>> CDS_DYNAMIC_ARCHIVE_MAGIC;
>>> 903 log_info(cds)("_magic expected: 0x%08x", expected_magic);
>>> 904 log_info(cds)(" actual: 0x%08x", _header->_magic);
>>> 905 FileMapInfo::fail_continue("The shared archive file has a bad
>>> magic number.");
>>> 906 return false;
>>> 907 }
>>> ...
>>>
>>> 964 if (is_static) {
>>> 965 if (_header->_magic != CDS_ARCHIVE_MAGIC) {
>>> 966 fail_continue("Incorrect static archive magic number");
>>> 967 return false;
>>> 968 }
>>>
>>> There are two checks for _header->_magic in
>>> FileMapInfo::init_from_file now but behave differently. The second one
>>> can be removed. The first check at line 901 should check the _magic
>>> value based on the 'is_static' flag:
>>>
>>> unsigned int expected_magic = is_static ? CDS_ARCHIVE_MAGIC :
>>> CDS_DYNAMIC_ARCHIVE_MAGIC;
>>> if (_header->_magic != expected_magic) {
>>> ...
>> I've made the above change.
>>> --------
>>>
>>> Most of the work now in FileMapInfo::init_from_file should really
>>> belong to FileMapInfo::validate_header. It would be cleaner to simply
>>> FileMapInfo::init_from_file to be the following and move the rest to
>>> FileMapInfo::validate_header. Thoughts?
>>>
>>> 888 bool FileMapInfo::init_from_file(int fd, bool is_static) {
>>> 889 size_t sz = is_static ? sizeof(FileMapHeader) :
>>> sizeof(DynamicArchiveHeader);
>>> 890 size_t n = os::read(fd, _header, (unsigned int)sz);
>>> 891 if (n != sz) {
>>> 892 fail_continue("Unable to read the file header.");
>>> 893 return false;
>>> 894 }
>>> 895 return true;
>>> }
>> The _file_offset will be based on the size_t n and some other fields
>> (_paths_misc_info, SharedBaseAddress) will be set at lines 953 - 976.
>> Also, there's the following check in validate_header():
>>
>> 1859 if
>> (!ClassLoader::check_shared_paths_misc_info(_paths_misc_info,
>> _header->_paths_misc_info_size, is_static)) {
>>
>> If the SharedPathsMiscInfo could be removed (JDK-8227370), then it is
>> possible that validate_header could be called within init_from_file. I
>> think we should defer this until JDK-8227370.
> Ok for deferring it.
>
> Best,
> Jiangli
>
>> updated webrev:
>>
>> http://cr.openjdk.java.net/~ccheung/8226406/webrev.01/
>>
>> thanks,
>>
>> Calvin
>>
>>> Best regards,
>>>
>>> Jiangli
>>>
>>>
>>> On Mon, Jul 8, 2019 at 10:25 AM Jiangli Zhou <jianglizhou at google.com> wrote:
>>>> Hi Calvin,
>>>>
>>>> On Mon, Jul 8, 2019 at 10:00 AM Calvin Cheung <calvin.cheung at oracle.com> wrote:
>>>>> Hi Jiangli,
>>>>>
>>>>> On 7/7/19 5:12 PM, Jiangli Zhou wrote:
>>>>>> Hi Calvin,
>>>>>>
>>>>>> Per our off-mailing-list email exchange from the previous code review
>>>>>> for https://bugs.openjdk.java.net/browse/JDK-8211723, I created
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8227370, 'Remove
>>>>>> SharedPathsMiscInfo'
>>>>> Thanks for filing the RFE.
>>>>>> . I think the crash caused by premature runtime accessing of
>>>>>> _paths_misc_info_size should be handled as part of JDK-8227370, rather
>>>>>> than further patching up the SharedPathsMiscInfo
>>>>> My current patch involves checking most the fields in
>>>>> CDSFileMapHeaderBase before accessing other fields. This part is
>>>>> applicable to other fields, not only to the _paths_misc_info_size. This
>>>>> bug existed for a while and I think it would be a good backport
>>>>> candidate for 11u. The patch for JDK-8211723 and the follow-up RFE
>>>>> JDK-8227370 are not necessary to be backported to 11u. I'd like to fix
>>>>> this bug first and then handle JDK-8227370 as a separate changeset.
>>>> That sounds like a good plan. A fix targeted for backporting should
>>>> have a clean-cut (less dependency) and controlled scope. Addressing
>>>> this incrementally in separate changesets is a suitable approach.
>>>>
>>>> I took a quick look over the weekend and noticed some issues with your
>>>> current patch. That's why I suggested to go with the complete removal
>>>> without spending extra effort on SharedPathsMiscInfo. I will need to
>>>> take a closer look and try to get back to you later today.
>>>>
>>>> Best regards,
>>>> Jiangli
>>>>
>>>>> thanks,
>>>>>
>>>>> Calvin
>>>>>
>>>>>> Thanks and regards,
>>>>>> Jiangli
>>>>>>
>>>>>> On Wed, Jul 3, 2019 at 5:59 PM Calvin Cheung <calvin.cheung at oracle.com> wrote:
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8226406
>>>>>>>
>>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8226406/webrev.00/
>>>>>>>
>>>>>>> This bug was found during a bootcycle build when a shared archive built
>>>>>>> by a 64-bit JDK version is used by a 32-bit JDK version. It is due to
>>>>>>> some of the important header fields such as the _jvm_ident was not
>>>>>>> checked prior to accessinng other fields such as the _paths_misc_info_size.
>>>>>>>
>>>>>>> This fix involves checking most the fields in CDSFileMapHeaderBase
>>>>>>> before accessing other fields.
>>>>>>>
>>>>>>> Testing: tiers 1-3.
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Calvin
>>>>>>>
More information about the hotspot-runtime-dev
mailing list