RFR: 8325108: POSIX map_memory_to_file calls release_memory incorrectly

[Gerard Ziemski]

On Thu, 1 Feb 2024 11:50:15 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

> Hi,
> 
> This PR fixes a small bug where `os::map_memory_to_file` attempts to unmap an incorrect mapping by calling `os::release_memory`. If NMT is turned on this will lead to a JVM crash, as the region was not previously mapped.
> 
> I'm fairly sure that this branch is actually dead code as `MAP_FIXED` is appended to the flags of the mmap call, and according to my man pages on mmap this will lead to mmap failing instead of giving another mapping to the address. However, I'd rather be on the safe side than sorry so I only do the minimal changes necessary to fix the bug.

Marked as reviewed by gziemski (Committer).

The fix itself looks good to me.

I do have some comments, however:

I see 2 call chains:


char* os::map_memory_to_file(size_t bytes, int file_desc, MEMFLAGS flag) {
  static char* map_memory_to_file(char* base = nullptr, size_t size = bytes, int fd = file_desc);


and


static char* reserve_memory(char* requested_address, const size_t size, const size_t alignment, int fd, bool exec) {
  static char* attempt_map_or_reserve_memory_at(char* base = requested_address, size_t size = size, int fd = fd, bool executable = exec) {
    char* os::attempt_map_memory_to_file_at(char* addr = base, size_t bytes = size, int file_desc = fd, MEMFLAGS flag) {
      char* os::pd_attempt_map_memory_to_file_at(char* requested_addr = addr, size_t bytes = bytes, int file_desc = file_desc) {
        if (result != nullptr)
        char* os::replace_existing_mapping_with_file_mapping(char* base = result, size_t size = bytes, int fd = file_desc) {
          static char* map_memory_to_file(char* base = base, size_t size = size, int fd = fd);


so it does not look like dead code to me.

Also, not 100% sure about the `MAP_FIXED` comment, my `man` page (on macOS) only talks about deleting previous mappings and that `Use of this option is discouraged.` though hotspot seems to be using `MAP_FIXED` in several places already.

-------------

PR Review: https://git.openjdk.org/jdk/pull/17669#pullrequestreview-2208187135
PR Comment: https://git.openjdk.org/jdk/pull/17669#issuecomment-2258771534