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

test: Add out-of-memory tests #773

Merged
merged 2 commits into from
Apr 9, 2021
Merged

test: Add out-of-memory tests #773

merged 2 commits into from
Apr 9, 2021

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented Mar 26, 2021

No description provided.

@codecov
Copy link

codecov bot commented Mar 26, 2021

Codecov Report

Merging #773 (f9cfe1f) into master (b8f6e50) will increase coverage by 0.05%.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##           master     #773      +/-   ##
==========================================
+ Coverage   99.19%   99.24%   +0.05%     
==========================================
  Files          77       78       +1     
  Lines       11936    11962      +26     
==========================================
+ Hits        11840    11872      +32     
+ Misses         96       90       -6     
Flag Coverage Δ
rust 99.87% <ø> (ø)
spectests 90.54% <ø> (ø)
unittests 99.20% <96.15%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
test/unittests/oom_test.cpp 96.15% <96.15%> (ø)
lib/fizzy/execute.cpp 99.53% <0.00%> (+0.81%) ⬆️

test/unittests/oom_test.cpp Outdated Show resolved Hide resolved
test/unittests/oom_test.cpp Outdated Show resolved Hide resolved
bool try_set_memory_limit(size_t size) noexcept
{
[[maybe_unused]] int err;
err = getrlimit(RLIMIT_AS, &orig_limit);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also have some stack tests using RLIMIT_STACK, i.e. having a known limit.

bool try_set_memory_limit(size_t size) noexcept
{
[[maybe_unused]] int err;
err = getrlimit(RLIMIT_AS, &orig_limit);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how RLIMIT_AS is different from RLIMIT_DATA (the latter being available on macos too):

       RLIMIT_AS
              This is the maximum size of the process's virtual memory
              (address space).  The limit is specified in bytes, and is
              rounded down to the system page size.  This limit affects
              calls to brk(2), mmap(2), and mremap(2), which fail with
              the error ENOMEM upon exceeding this limit.  In addition,
              automatic stack expansion fails (and generates a SIGSEGV
              that kills the process if no alternate stack has been made
              available via sigaltstack(2)).  Since the value is a long,
              on machines with a 32-bit long either this limit is at
              most 2 GiB, or this resource is unlimited.
       RLIMIT_DATA
              This is the maximum size of the process's data segment
              (initialized data, uninitialized data, and heap).  The
              limit is specified in bytes, and is rounded down to the
              system page size.  This limit affects calls to brk(2),
              sbrk(2), and (since Linux 4.7) mmap(2), which fail with
              the error ENOMEM upon encountering the soft limit of this
              resource.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I have found is that RLIMIT_DATA may look more what we want (at least is more concrete). And it also seems to be working fine. And documentation says it also applies to mmap(). However, before (Linux 4.7) the limit could be bypassed by malloc using mmap for big allocations (as our case).

Therefore, the RLIMIT_AS seems to be "safer" or "more portable" option.

BTW, @axic should try this on macos (currently disabled).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, @axic should try this on macos (currently disabled).

I can once it is switched to RLIMIT_DATA.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is RLIMIT_DATA but it does not apply to mmap on macos. Please test it and report back.

Copy link
Member

@axic axic Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this on macOS. Added __APPLE__ next to __linux__. Everything compiles fine, but it "does not work": the syscalls succeed with 0, but the allocation is not terminated.

  1. Dumped limits, and the new limits are reflected by getrlimit. So it definitely "works".
  2. Tried setting not only cur, but max. No change.
  3. Apparently RLIMIT_AS (an alias to RLIMIT_RSS) is also available on macOS, just not part of the man page. (ref). Tried, compiles, but no change.
  4. It seems that setrlimit/getrlimit is properly implemented in macOS (ref), but RLIMIT_DATA or RLIMIT_AS is actually not used by anything later on. RLIMIT_FSIZE/CPU/NPROC/NOFILE/CORE/STACKMEMLOCK are used in places, so those may be enforced.

See also https://stackoverflow.com/a/6169866 for potential solutions.

Note: launchd is some system process, but looking deeper in the end it still uses setrlimit/getrlimit without any special magic. So I think neither of that does anything.

test/unittests/oom_test.cpp Outdated Show resolved Hide resolved
test/unittests/oom_test.cpp Outdated Show resolved Hide resolved
@axic
Copy link
Member

axic commented Apr 9, 2021

Can we squash some of these commits?

@chfast chfast merged commit a1a1b61 into master Apr 9, 2021
@chfast chfast deleted the oom_tests branch April 9, 2021 19:16
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.

3 participants