RFR(S) 8226406: JVM fails to detect mismatched or corrupt CDS archive
Calvin Cheung
calvin.cheung at oracle.com
Wed Jul 10 07:28:53 UTC 2019
On 7/9/19 5:32 PM, Jiangli Zhou wrote:
> On Mon, Jul 8, 2019 at 11:35 PM Calvin Cheung <calvin.cheung at oracle.com> wrote:
>>
>> On 7/8/19 4:45 PM, Jiangli Zhou wrote:
>>> -#define CURRENT_CDS_ARCHIVE_VERSION 5
>>> +#define CURRENT_CDS_ARCHIVE_VERSION 6
>>>
>>> I would also suggestion to not do the above change in this bug fix
>>> since that would make all older versions to use '6' when backported
>>> (unless hand merge is involved).
>> Since the _jvm_ident field has been moved to a different location, I
>> think the CURRENT_CDS_ARCHIVE_VERSION should be updated. Even if the
>> version stays the same, shared archive created by an older version of
>> JVM cannot be used by the current JVM version.
> Can you please clarify the reason for moving the field?
One advantage is that there's currently a 4-byte gap between _version
and _space. Placing the _jvm_ident field after _version, the first 4
fields will be 4-byte aligned. Anyway, I've reverted the change in my
latest webrev.
57 struct CDSFileMapHeaderBase {
58 unsigned int _magic; // identify file type
59 int _crc; // header crc checksum
60 int _version; // must be
CURRENT_CDS_ARCHIVE_VERSION
61 struct CDSFileMapRegion _space[NUM_CDS_REGIONS];
62 };
>
> It's confusing for all different JDK versions to have the same
> CURRENT_CDS_ARCHIVE_VERSION but with significantly different archive
> layouts.
I've checked the change history of the CURRENT_CDS_ARCHIVE_VERSION. Last
update was for the following bug fix:
8208658: Make CDS archived heap regions usable even if compressed oop
encoding has changed
Since there were 2 fields added to the header for the dynamic CDS
archive, the version should have been updated again. Should I file a bug
to update the version?
thanks,
Calvin
>
> Best,
> Jiangli
>> thanks,
>>
>> Calvin
>>
>>> Thanks,
>>> Jiangli
>>>
>>> On Mon, Jul 8, 2019 at 4:38 PM Jiangli Zhou <jianglizhou at google.com> 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.
>>>>
>>>> --------
>>>>
>>>> 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?
>>>>
>>>> --------
>>>>
>>>> - 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) {
>>>> ...
>>>>
>>>> --------
>>>>
>>>> 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;
>>>> }
>>>>
>>>> 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