8283469: Don't use memset to initialize members in FileMapInfo and fix memory leak

[zhengyu123]

FileMapInfo uses memset in its constructor to initialize data member, it is generally a bad idea. It can break things badly if adds a new member with a non-trivial constructor and memset is not suitable.

Besides, it leaks malloc'd _header member.

Test:

• hotspot_cds
• tier1
• tier2
• runtime/cds/DeterministicDump.java

---

Progress

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

Issue

• JDK-8283469: Don't use memset to initialize members in FileMapInfo and fix memory leak

Reviewers

• Ioi Lam (@iklam - Reviewer)
• Kim Barrett (@kimbarrett - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7899

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

Using diff file

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

[bridgekeeper]

👋 Welcome back zgu! 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]

@zhengyu123 The following label will be automatically applied to this pull request:

• hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (b82eb1ef)

[dholmes-ora]

Isn't this one of the things we have to guarantee was zeroed at initialization to produce deterministic shared archive contents?

David

[iklam]

Isn't this one of the things we have to guarantee was zeroed at initialization to produce deterministic shared archive contents?

David

FileMapInfo is derived from CHeapObj<mtInternal> so it's allocated by os::malloc, which zeros the memory when DumpSharedSpaces == true.

[iklam]

LGTM. Please run all CDS tests, including DeterministicDump.java.

[openjdk]

@zhengyu123 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8283469: Don't use memset to initialize members in FileMapInfo and fix memory leak

Reviewed-by: iklam, kbarrett

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 76 new commits pushed to the master branch:

• c2c0cb2: 8282668: HotSpot Style Guide should permit unrestricted unions
• b0daf70: 8263134: HotSpot Style Guide should disallow inheriting constructors
• c587b29: 8283720: ProblemList java/time/test/java/time/TestZoneOffset.java
• d5f9059: 8283695: [AIX] Build failure due to name conflict in test_arguments.cpp
• f520b4f: 8283668: Update IllegalFormatException to use sealed classes
• 5ca8b91: 8283713: [BACKOUT] Unexpected TypeElement in ANALYZE TaskEvent
• 7bac0a8: 8283681: Improve ZonedDateTime offset handling
• 2600f99: 8282685: fileToEncodedURL_[name|signature] symbols are unused
• e97cf15: 8283691: Classes in java.security still reference deprecated classes in spec
• cb012a5: 8283648: Improve the snippet "file not found" message.
• ... and 66 more: https://git.openjdk.java.net/jdk/compare/14b9e80b8adcc0ab0634357f5a7c25f24fd6808c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[zhengyu123]

Friendly ping! Might I get a second review.

Thanks

[kimbarrett]

Looks good.

memxxx of an object is only correct under specific circumstances. A
case where it's not is when the object has virtual functions, which
FileMapInfo does in debug builds, because CHeapObj derives from
AllocatedObj in debug builds and that class has virtual functions.
Clearing the entire object is zeroing out vtable info, and trying to
call those functions will almost certainly crash. So the current code
is already invalid; there's no need to wait for future members with
non-trivial constructors.

It looks like FileMapInfo might really a union of two cases, the
static and !static case, with some members associated with only one or
the other case. In the future it might be worth considering whether
that could be made more formal.

[zhengyu123]

Thanks for the reviews, @iklam @kimbarrett

[zhengyu123]

/integrate

[openjdk]

Going to push as commit d6fa8b0.
Since your change was applied there have been 81 commits pushed to the master branch:

• 8567266: 8283683: Make ThreadLocalRandom a final class
• f4eaa16: 8283728: jdk.hotspot.agent: Wrong location for RISCV64ThreadContext.java
• cdef087: 8283727: P11KeyGenerator has import statement with two semicolons after JDK-8267319
• 7f12537: 8283558: Parallel: Pass PSIsAliveClosure to ReferenceProcessor constructor
• 66f1da1: 8281222: ciTypeFlow::profiled_count fails "assert(0 <= i && i < _len) failed: illegal index"
• c2c0cb2: 8282668: HotSpot Style Guide should permit unrestricted unions
• b0daf70: 8263134: HotSpot Style Guide should disallow inheriting constructors
• c587b29: 8283720: ProblemList java/time/test/java/time/TestZoneOffset.java
• d5f9059: 8283695: [AIX] Build failure due to name conflict in test_arguments.cpp
• f520b4f: 8283668: Update IllegalFormatException to use sealed classes
• ... and 71 more: https://git.openjdk.java.net/jdk/compare/14b9e80b8adcc0ab0634357f5a7c25f24fd6808c...master

Your commit was automatically rebased without conflicts.

[openjdk]

@zhengyu123 Pushed as commit d6fa8b0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.