RFR(S) 8226406: JVM fails to detect mismatched or corrupt CDS archive

Tue Jul 9 06:34:28 UTC 2019

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.

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
>>>>>>