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

build runner: actually limit the peak RSS rather than enforcing it post-hoc #14953

Open
Tracked by #14647
andrewrk opened this issue Mar 16, 2023 · 2 comments
Open
Tracked by #14647
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. zig build system std.Build, the build runner, `zig build` subcommand, package management
Milestone

Comments

@andrewrk
Copy link
Member

Extracted from #14647.

Currently, it is enforced post-hoc here:

zig/lib/std/Build/Step.zig

Lines 176 to 182 in b4d58e9

if (s.max_rss != 0 and s.result_peak_rss > s.max_rss) {
const msg = std.fmt.allocPrint(arena, "memory usage peaked at {d} bytes, exceeding the declared upper bound of {d}", .{
s.result_peak_rss, s.max_rss,
}) catch @panic("OOM");
s.result_error_msgs.append(arena, msg) catch @panic("OOM");
return error.MakeFailed;
}

Instead, this issue proposes to enforce it using operating system APIs.

I looked briefly into this, and it looked like a giant pain in the ass to do this, at least on Linux.

@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. zig build system std.Build, the build runner, `zig build` subcommand, package management labels Mar 16, 2023
@andrewrk andrewrk added this to the 0.13.0 milestone Mar 16, 2023
@matu3ba
Copy link
Contributor

matu3ba commented Mar 17, 2023

For Linux (Unixoids):

Afaiu, one way to go is to read /proc/self/status https://stackoverflow.com/questions/1558402/memory-usage-of-current-process-in-c/47531152#47531152. Also, fopen them may fail and, afaiu, they are racy on reading.

The alternatives are to 1. invoke every time getrusage as syscall, 2. track all allocations synchronized in memory (meaning atomic access to write new memory usage on every "page allocation and free") , 3. use cgroups, 4. landlock offers an api to the memory allocator https://docs.kernel.org/userspace-api/landlock.html

In an ideal world, we could ask the Kernel upfront for memory up to a process limit in the syscall similar to how io_uring bundles multiple info together for better perf.
Option 1+2 offer no protection against mailicious use/bypassing the Zig allocator interface.
Option 3+4 dont work on Windows and will likely not work in future on Windows.

Please let me know, if I forgot anything.

APPENDUM: On Windows there is win32 with GetProcessMemoryInfo syscall, which probably uses NtQuerySystemInformation to get the used memory.

APPENDUM2: From quick glimpse, it looks like the wait statistics are used, but getrusage approach has not been implemented.

@matu3ba
Copy link
Contributor

matu3ba commented Jan 12, 2024

Must have not read properly and I need this for a thing (root requirement ok though).

Limiting works rather simple, but we would need a declarative table for it on Posix to talk to the child process after clone/fork before it does exec*.
If you think the complexity is worth it, then I'd lean towards an environment variable specifying tcp connection or pipe and closing it before execve. See https://stackoverflow.com/questions/7889594/setrlimit-on-another-process.
However, this would push us into "handle the leaking file descriptors territory" on Posix.

I'll soonish get to this:

  • job api in windows limitation done. its kind of a clusterfuck, so I'm very much unsure if we want all of that additional api surface in libstd. We might want to limit it or use the ntdll calls instead. On the other hand there is no other way to limit memory consumption.
  • still working on tests (processes really terminated, memory limits correct etc), since I'm concerned if pinning if necessary and windows behaves as advertised
  • posix api boils down to soft limit and exception in descendant or hard limit and SIGKILL the descendant by kernel

https://devblogs.microsoft.com/oldnewthing/20230209-00/?p=107812 of course the error free way uses the extended api on Windows. Windows offers apparently no other way to restrict process memory, see https://superuser.com/questions/1263090/is-it-possible-to-limit-the-memory-usage-of-a-particular-process-on-windows.
However, I suspect by some evidence, that one needs to to some more involved setup as it was reported getting correct time and memory usage requires to pin things or they are wrong.

matu3ba added a commit to matu3ba/sandboxamples that referenced this issue Jan 14, 2024
- Include related examples to check, if processes were terminatd.
- SPDX identifiers, if applicable like in chid_process.
- This does not add common limits yet for solving in child_process
  ziglang/zig#14953, since the POSIX
  implementation is missing and might affect design.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. zig build system std.Build, the build runner, `zig build` subcommand, package management
Projects
None yet
Development

No branches or pull requests

2 participants