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

Jiangli Zhou jianglizhou at google.com
Wed Jul 10 20:26:21 UTC 2019


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