RFR(S) 8226406: JVM fails to detect mismatched or corrupt CDS archive
Calvin Cheung
calvin.cheung at oracle.com
Wed Jul 10 21:54:42 UTC 2019
Thanks for taking another look.
Calvin
On 7/10/19 1:26 PM, Jiangli Zhou wrote:
> The updates look ok. We do have cdsoffsets.cpp in JDK 11 (cds.h
> doesn't exist in JDK 11), so it shouldn't cause backport issue.
>
> Best,
> Jiangli
>
> On Wed, Jul 10, 2019 at 12:14 AM Calvin Cheung <calvin.cheung at oracle.com> wrote:
>>
>> 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