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

Add cross-platform memory map abstraction and use it in libfuzzer. #21083

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

gcoakes
Copy link
Contributor

@gcoakes gcoakes commented Aug 15, 2024

This is a collection of commits working towards both #20987 and #20803. They implement the following:

  • Windows extern definitions for CreateFileMapping and MapViewOfFile.
  • A cross-platform abstraction for mapping files into virtual memory.
  • Using that abstraction in libfuzzer's seen_pcs field.

There is still work that needs to happen for both of those issues.

@gcoakes
Copy link
Contributor Author

gcoakes commented Aug 15, 2024

Note for reviewers, I removed MemoryMappedList from libfuzzer because I realized the current usage never grows the file. MemoryMappedList seems like it was added in preparation for using the same type to represent both fixed-size, memory-mapped files and dynamic-sized, memory-mapped files.

I think that is better handled by two separate types: (1) std.fs.MemoryMap and (2) std.fs.MemoryMapGrowable (name is flexible). I had both implemented in my second commit, but I realized std.fs.MemoryMapGrowable would not actually be used until working on memory-mapping the Fuzzer.input field (ziglang/zig/issues#20803). So, I stashed those changes and should have another PR implementing that.

@gcoakes
Copy link
Contributor Author

gcoakes commented Aug 15, 2024

Checks are failing for Linux due to non-native operating systems: WASI and Windows. I've been testing with zig build test-std -Dskip-non-native -Dskip-release as was recommended in the wiki.

WASI is a simple matter of producing a compile error when it is attempted to be used while targeting WASI. WebAssembly/WASI#304

Windows seems to be a Wine specific issue since it passes on my Windows development machine and CI reports success on Windows. I need to get Wine testing working on my Linux development machine.

Moving to draft until I can resolve those. I'll fork this branch to a non-PR one so that I can not waste your CI resources while working on that.

These functions are wrappers over NtDll's `NtCreateSection` and
`NtMapViewOfSection`. They convert Zig-native ergonomics into calls to
the wrapped functions.
This new type provides a cross platform memory mapping API which
encompasses the common subset of both POSIX and Windows APIs.
@gcoakes
Copy link
Contributor Author

gcoakes commented Aug 19, 2024

CI failed for aarch64 MacOS Release only (Debug passed). The error message was saying something about a missing feature. That makes me think it is unrelated to my changes.

This branch was several commits behind master. I rebased onto the latest master in the hopes of resolving the CI issue. In any case, I do not have access to a MacOS aarch64 machine to test it locally.

@gcoakes gcoakes marked this pull request as ready for review August 19, 2024 05:35
@gcoakes
Copy link
Contributor Author

gcoakes commented Aug 19, 2024

The CI failures appear spurious and unrelated to the changes here. This time it is on a different OS panicking on a debug assertion in a totally unrelated part of code:

error: thread 790374 panic: reached unreachable code
/home/ci/actions-runner11/_work/zig/zig/lib/std/debug.zig:399:14: 0x4ce6fe7 in assert (zig)
    if (!ok) unreachable; // assertion failure
             ^
/home/ci/actions-runner11/_work/zig/zig/lib/std/array_hash_map.zig:1959:11: 0x4ebd943 in capacityIndexType (zig)
    assert(bit_index <= 32);

I don't see how this patchset could even cause a regression inside the compiler since the newly added standard library APIs are not used in the compiler.

@gcoakes gcoakes requested a review from squeek502 August 23, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants