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

Jiangli Zhou jianglizhou at google.com
Mon Jul 8 23:38:51 UTC 2019


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