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

Tue Jul 9 06:29:16 UTC 2019

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

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

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