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

[Jiangli Zhou]

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

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