RFR(S) 8226406: JVM fails to detect mismatched or corrupt CDS archive
Jiangli Zhou
jianglizhou at google.com
Wed Jul 10 21:06:55 UTC 2019
On Wed, Jul 10, 2019 at 12:29 AM Calvin Cheung <calvin.cheung at oracle.com> wrote:
>
>
> 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?
Filing a bug sounds good.
Best,
Jiangli
>
> 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