Cleanup most usages of Windows file mapping

[huoyaoyuan]

The remaining cases are superpmi (included in #121367) and PEImage, which involves LoadLibrary and we would want more future-proof abstraction.

All the cases included in this PR are transitively used by ilasm/ildasm. For stgio, I'm doing simple ifdef's on OS api, since it's too complicated around different I/O models, and we would get rid of it with new metadata library.

[AaronRobinsonMSFT]

Is there any concern with the path or desiredAddress being null? I'd assume that asserts are warranted. Same for Windows implementation.

[huoyaoyuan]

Has it been a practice? Shouldn't the OS fs call reliably reject invalid string?

[AaronRobinsonMSFT]

path is used below in u16_strlen. That doesn't and shouldn't check for NULL. Unclear on minipal_get_length_utf16_to_utf8.

[huoyaoyuan]

Added assert. For Windows version, it is directly passed to CreateFileW which should reject null.

[janvorli]

I wonder if at all call sites the possibility of badalloc exception is correctly handled. It seems it may make sense to use new (nothrow) here and return nullptr if the allocation failed so that all the error paths are handled in a similar way.

[huoyaoyuan]

This pattern has been used in various places. If we want to handle badalloc, it should probably be updated altogether.

[huoyaoyuan]

All tests are passing now, and I should have responded to all comments.

[janvorli]

A nit - I am not sure what the dn- filename prefix means. @AaronRobinsonMSFT I can see that you have originally added the dn-u16.cpp file with this naming convention, so you probably know the meaning.

[AaronRobinsonMSFT]

The dn is for "dotnet". It is akin to the PAL_ prefix we've historically used and meant only to be used when needed to disambiguate something. I was hoping it would evolve into a pattern that was obvious. Unclear if that remains the case. I've no concern with dropping it at this point. I do find it useful for headers so I know where they are coming from, but for TUs it is probably unnecessary.

[janvorli]

Why was this changed to uint32_t?

[huoyaoyuan]

The caller already restricts its value to uint32. Doing the cast at caller making it more consistent with what's supported in MemoryMappedFile::Open.

[janvorli]

It seems to me that the new memory mapping APIs we are adding to minipal would be more future proof if they were not artificially limited to 4GB large files. While it is currently used only for max 4GB mappings, it may get used in the future for something else.

[huoyaoyuan]

I wonder whether it would be treated as a future-oriented API, or just temporary minimal adapters for migrating legacy components. For the use cases: ilasm/idasm is being migrated away, stgio is complicated from the old days and may go away with new DNMD, and tools like metainfo can simply switch to ReadAllBytes-like approach.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "4260f9f37257066d74ff1ee91bfc1e60ef221b2f",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "b88ce03bc28304f2b6cfd27c29923e142a9040c6",
  "last_reviewed_commit": "4260f9f37257066d74ff1ee91bfc1e60ef221b2f",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "b88ce03bc28304f2b6cfd27c29923e142a9040c6",
  "last_recorded_worker_run_id": "29677690748",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "4260f9f37257066d74ff1ee91bfc1e60ef221b2f",
      "review_id": 4730527939
    }
  ]
}

[github-actions]

Holistic Review

Motivation: Reducing direct dependence on Win32 file-mapping APIs (CreateFileMapping/MapViewOfFile) in the ilasm/ildasm/metadata toolchain is a reasonable, well-scoped cleanup that advances the PAL-removal effort. The problem is real and the direction matches the stated future goal of a portable metadata library.

Approach: Introducing a small cross-platform MemoryMappedFile minipal abstraction plus HOST_WINDOWS/HOST_UNIX #ifdef branches in stgio.cpp is consistent with how this area already handles portability. However, several of the hand-written Unix branches contain correctness bugs (unsigned/ssize_t comparisons that mask errors and drop out-parameters), and one refactor silently drops PID-substitution behavior in the stress log. These need fixing before merge.

Summary: ⚠️ Needs Changes. The abstraction is sound and the mechanical conversions are mostly faithful, but there are concrete regressions: (1) ReadFromDisk can never observe a read failure and never updates *pcbRead; (2) WriteToDisk mishandles write errors/short writes and doesn't propagate the byte count; (3) the stress-log conversion passes the un-substituted filename, breaking per-process log paths. See inline comments and the additional observations below.

---

Detailed Findings

❌ Correctness — Unix ReadFromDisk/WriteToDisk error handling (inline)

See inline comments on src/coreclr/md/enc/stgio.cpp. Both use ULONG locals to hold the ssize_t result of read/write, so the -1 error sentinel wraps to UINT_MAX and the error/short-transfer paths behave incorrectly, and the actual transferred count is not written back to the caller's *pcbRead/*pcbWritten. The Windows branches propagate both the failure and the count.

❌ Correctness — Stress log drops PID substitution (inline)

See inline comment on src/coreclr/utilcode/stresslog.cpp line 196. logFilenameReplaced is still computed but no longer used; the original logFilename is passed to MemoryMappedFile::OpenRW.

⚠️ Cross-platform semantics — desiredAddress is only a hint on Unix

src/coreclr/minipal/Unix/memmap.cpp calls mmap(desiredAddress, ...) without MAP_FIXED, so desiredAddress is merely a hint and the mapping may land elsewhere with no error. The Windows path uses MapViewOfFileEx, which fails if the requested base address is unavailable. The stress-log caller passes MEMORY_MAPPED_STRESSLOG_BASE_ADDRESS and elsewhere assumes the header lives at that fixed base. If a fixed base is required for correctness on Unix, this needs MAP_FIXED (or an explicit post-check that the returned address matches); if the address is truly advisory, that difference from the Windows contract is worth a comment. Please confirm which behavior is intended.

⚠️ Resource/robustness — Unix memmap.cpp allocation and zero-length map

In OpenImpl the UTF-8 path buffer is allocated with new char[...] and freed with delete[] only on the success path before the goto Fail chain begins, which is fine, but note that if st.st_size == 0 in read mode, mmap with length 0 returns EINVAL and Open fails; the previous Windows code could map a zero-length file. Confirm no caller relies on mapping empty files (the stgio.cpp read path already rejects size 0, so this is likely fine for the metadata cases, but the standalone MemoryMappedFile::Open used by ildasm/ilasm/mdobj does not pre-check).

💡 Style — #else // empty comment

src/coreclr/md/inc/stgio.h: the #else // on the IsMemoryMapped() branch has an empty/typo comment where the sibling branches use // HOST_WINDOWS. Minor consistency nit.

✅ Mechanical conversions in consumers look faithful

The PELoader, MappedFileStream, and mdobj.cpp conversions to the new MemoryMappedFile*/CreateMappedFile abstraction correctly replace the manual CreateFile/CreateFileMapping/MapViewOfFile/UnmapViewOfFile/CloseHandle sequences with construct-and-delete, and the removed getHFile()/open(const char*)/GetMapViewOfFile members appear to have no remaining callers in the diff. The >4GB guards and the getFileSize() replacement for SafeGetFileSize are reasonable given the stated "no files >4GB" constraint.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 174.6 AIC · ⌖ 11.9 AIC · ⊞ 10K

[github-actions]

❌ read error is never detected, and *pcbRead is not updated.

cbRead is declared as ULONG (unsigned). read() returns ssize_t; on error it returns -1, which becomes UINT_MAX when stored in cbRead, so (cbRead = read(...)) >= 0 is always true and the error path (return HRESULTFromErr(errno);) is unreachable. The Windows branch reports failure via ReadFile, so this is a behavioral regression on Unix.

Additionally, the Windows ReadFile call writes the byte count into *pcbRead, but this branch only assigns the local cbRead and never propagates it to *pcbRead. Callers such as Read() do m_cbOffset += *pcbRead; after ReadFromDisk(..., pcbRead), so they consume a stale/uninitialized value.

Suggest capturing the result in a signed type and writing back, e.g.:

ssize_t cbReadActual = read(m_fd, pbBuff, cbBuff);
if (cbReadActual >= 0)
{
    *pcbRead = (ULONG)cbReadActual;
    return (S_OK);
}
return HRESULTFromErr(errno);

[github-actions]

⚠️ Signedness/short-write mismatch in WriteToDisk. cbWritten is ULONG; write() returns ssize_t. On error write returns -1 which becomes UINT_MAX and compares unequal to cbWrite, so hr is set — but errno may not be meaningful in that comparison path, and more importantly a partial write (a legitimate short write) is treated as an error while *pcbWritten is left holding the wrong value. The Windows branch propagates the actual written count via WriteFile's pcbWritten. Consider assigning the actual count back into *pcbWritten and using a signed temporary for the comparison so a genuine error (-1) is distinguished from a short write.

[github-actions]

❌ PID substitution is dropped — the wrong filename is used. The code still computes logFilenameReplaced via ReplacePid(logFilename, logFilenameReplaced, MAX_PATH) on the line above, but this call passes the original logFilename to MemoryMappedFile::OpenRW instead of logFilenameReplaced. The previous Windows code passed logFilenameReplaced to WszCreateFile. As a result the %p/PID token in the configured stress-log path is no longer expanded, so all processes collide on the same unexpanded filename (and logFilenameReplaced becomes a dead variable). Pass logFilenameReplaced here.