Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8273152: Store base archive path in CDSFileMapHeaderBase #5584

Closed
wants to merge 4 commits into from

Conversation

yminqi
Copy link
Contributor

@yminqi yminqi commented Sep 20, 2021

Store base archive path offset in CDSFileMapHeaderBase (which should be invariant in evolving versions) so we can read the base archive name reliably from dynamic archive even with variant header sizes over versions.

tests: tier1-4

Thanks
Yumin


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8273152: Store base archive path in CDSFileMapHeaderBase

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5584/head:pull/5584
$ git checkout pull/5584

Update a local copy of the PR:
$ git checkout pull/5584
$ git pull https://git.openjdk.java.net/jdk pull/5584/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5584

View PR using the GUI difftool:
$ git pr show -t 5584

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5584.diff

@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Sep 20, 2021

/label add hotspot-runtime

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 20, 2021

👋 Welcome back minqi! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the hotspot-runtime label Sep 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 20, 2021

@yminqi
The hotspot-runtime label was successfully added.

@yminqi yminqi marked this pull request as ready for review Sep 20, 2021
@openjdk openjdk bot added the rfr label Sep 20, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 20, 2021

Webrevs

@@ -64,6 +64,11 @@ struct CDSFileMapHeaderBase {
unsigned int _magic; // identify file type
int _crc; // header crc checksum
int _version; // must be CURRENT_CDS_ARCHIVE_VERSION
int _base_archive_path_offset; // This field is present with (_version >= 12). It's declaration
Copy link
Member

@iklam iklam Sep 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CURRENT_CDS_ARCHIVE_VERSION in this file should also be changed to 12.

Typo: It's declaration -> Its declaration

For safety, I think we need an extra field, and also change _base_archive_path_offse to unsigned:

  int          _version; 
+ unsigned int _header_size;       // total size of the header, in bytes
! unsigned int _base_archive_path_offset;

We need new test cases for safety checks:

  1. _header_size is larger than the size of the archive file.
  2. _base_archive_path_offset points is not zero for static archive.
  3. For dynamic archive: _base_archive_path_offset points beyond _header_size
  4. For dynamic archive: (((char*)this) + _base_archive_path_offset) does not point to a zero-terminated string. I.e., there is no zero byte between (((char*)this) + _base_archive_path_offset) and the end of the header.

Also, we should add something like this in the very beginning of the structure. E.g., we don't want someone to add a field in the middle of them.

// The declaration of the following 5 fields must never be changed
// in any future versions of HotSpot

return false;
}
}

Copy link
Member

@iklam iklam Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check from 1144-1158 should be done inside FileMapInfo::check_archive(). This function (FileMapInfo::init_from_file) is called after we have successfully loaded the header of an archive created by the current JDK. However, -- with JDK-8261455, we will need to read the headers of an archive created by a different JDK version, so we need to do the check at the very beginning, when we are examining the magic value, etc.

Also, the base archive's name should be considered as part of the header, so header()->header_size() should include the size of the base archive's name as well. Otherwise, we won't be able to read the entire header, including the base archive name, of an archive created by a different JDK version.

The check for header()->base_archive_path_offset() needs to be expanded:

  • it must be smaller than header()->header_size()
  • it must point to a valid 0-terminated string -- the string cannot run past the end of the header.

Copy link
Contributor Author

@yminqi yminqi Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The base archive name size should be included in CDSFileMapHeaderBase, not the base archive name itself. If the name as a part of header size, it will complicate the code. We want to read the base archive name from the archive file, this should be done by only reading CDSFileMapHeaderBase from the archive file to get enough information.

@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Sep 29, 2021

The patch will be undergo more modification to the header file. A new patch will include the new design and this is closed without further work.

@yminqi yminqi closed this Sep 29, 2021
@yminqi yminqi deleted the jdk-8273152 branch Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime rfr
2 participants