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

Fix PortSysinfoTest failure when running as root #6513

Merged
merged 2 commits into from
May 16, 2022

Conversation

EricYangIBM
Copy link
Contributor

PortSysinfoTest.sysinfo_test_sysinfo_set_limit_FILE_DESCRIPTORS fails
when running as root. Make sure setrlimit does not lower the hard limit
below the soft limit as this is not allowed, even as root.

Signed-off-by: Eric Yang [email protected]

@EricYangIBM
Copy link
Contributor Author

@babsingh fyi

@babsingh
Copy link
Contributor

Same OSX failures, as seen in #6494 (comment), occur in this PR:

31: [----------] 19 tests from PortSockTest
31: /Users/runner/work/1/s/fvtest/porttest/omrsockTest.cpp:1209: Failure
31:       Expected: rc
31:       Which is: 1
31: To be equal to: 2
31: [  FAILED  ] PortSockTest.poll_functionality_basic (1111 ms)
31: /Users/runner/work/1/s/fvtest/porttest/omrsockTest.cpp:51: Failure
31:       Expected: privateOmrPortLibrary->sock_bind(privateOmrPortLibrary, *serverSocket, serverSockAddr)
31:       Which is: -506
31: To be equal to: 0
31: /Users/runner/work/1/s/fvtest/porttest/omrsockTest.cpp:1259: Failure
31:       Expected: privateOmrPortLibrary->sock_accept(privateOmrPortLibrary, serverSocket, &connectedServerSockAddr, &connectedServerSocket)
31:       Which is: -20
31: To be equal to: 0
31: [  FAILED  ] PortSockTest.poll_functionality_many_sockets (5 ms)
31: [----------] 19 tests from PortSockTest (1121 ms total)
31: 
31: [==========] 236 tests from 20 test cases ran. (110437 ms total)
31: [  PASSED  ] 234 tests.
31: [  FAILED  ] 2 tests, listed below:
31: [  FAILED  ] PortSockTest.poll_functionality_basic
31: [  FAILED  ] PortSockTest.poll_functionality_many_sockets

Opened #6516 to document the above failures.

@babsingh
Copy link
Contributor

jenkins build all

@babsingh
Copy link
Contributor

babsingh commented May 13, 2022

I found two Linux man pages for setrlimit:

  1. https://linux.die.net/man/2/setrlimit (2 - System calls)

A privileged process (under Linux: one with the CAP_SYS_RESOURCE capability) may make arbitrary changes to either limit value.

  1. https://linux.die.net/man/3/setrlimit (3 - Library calls)

This doc does not mention the above statement, but it mentions the below statement to support the current changes:

A process may (irreversibly) lower its hard limit to any value that is greater than or equal to the soft limit. Only a process with appropriate privileges can raise a hard limit.

Which man page applies to us?

Make sure setrlimit does not lower the hard limit below the soft limit as this is not allowed, even as root.

Is the above statement also valid for other Unix systems: mac, AIX and zOS?

fvtest/porttest/si.cpp Outdated Show resolved Hide resolved
@EricYangIBM
Copy link
Contributor Author

A privileged process (under Linux: one with the CAP_SYS_RESOURCE capability) may make arbitrary changes to either limit value.

I don't think this should apply, it makes no sense to be able to raise the soft limit above the hard limit. Also I can't find this statement in any other documentation.

In the same page:

EINVAL
The value specified in resource is not valid; or, for setrlimit() or prlimit(): rlim->rlim_cur was greater than rlim->rlim_max.

Osx:
https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/setrlimit.2.html

[EINVAL] The specified limit is invalid (e.g., RLIM_INFINITY or lower than rlim_cur).
... setrlimit() now returns with errno set to EINVAL in places that historically
succeeded. It no longer accepts "rlim_cur = RLIM_INFINITY" for
RLIM_NOFILE. Use "rlim_cur = min(OPEN_MAX, rlim_max)".

Aix:
https://www.ibm.com/docs/en/aix/7.3?topic=g-getrlimit-getrlimit64-setrlimit-setrlimit64-vlimit-subroutine

A calling process can raise or lower its own soft limits, but it cannot raise its soft limits above its hard limits. A calling process must have root user authority to raise a hard limit.

Zos:
https://www.ibm.com/docs/en/zos/2.5.0?topic=functions-setrlimit-control-maximum-resource-consumption

The hard limit may be lowered to any value that is greater than or equal to the soft limit. The hard limit can be raised only by a process which has superuser authority. Both the soft limit and hard limit can be changed by a single call to setrlimit().

fvtest/porttest/si.cpp Outdated Show resolved Hide resolved
@babsingh
Copy link
Contributor

jenkins build all

@babsingh
Copy link
Contributor

jenkins build zos

@jdmpapin
Copy link
Contributor

This test resets the soft limit again at the end, claiming it may have been lowered implicitly to stay within the hard limit:

/* restore original soft limit
   The soft limit is always <= the hard limit. If the hard limit is lowered to below the soft limit and
   then raised again the soft limit isn't automatically raised. */

Is that possible? (Or was it ever?) It doesn't seem to be consistent with the behaviour we now expect with this change

The root path will now explicitly lower the soft limit though, so I guess it still needs to be restored in that case

@EricYangIBM
Copy link
Contributor Author

Not sure about that comment. I don't think the soft limit is automatically lowered with the hard limit since setrlimit() sets both at the same time (with a struct) and EINVAL is returned when rlim->rlim_cur was greater than rlim->rlim_max.
The existing code finalSoftLimit = (originalSoftLimit > newHardLimit)? newHardLimit: originalSoftLimit; does seem to imply that it is possible to set the hard limit below the soft limit but it's probably a misunderstanding

@jdmpapin
Copy link
Contributor

OK, then I think it would be good to move the restoration into the root path, fix the comment, eliminate finalSoftLimit, and maybe verify at the end of the non-root path that the soft limit is still originalSoftLimit

PortSysinfoTest.sysinfo_test_sysinfo_set_limit_FILE_DESCRIPTORS fails
when running as root. Make sure setrlimit does not lower the hard limit
below the soft limit as this is not allowed, even as root.

Signed-off-by: Eric Yang <[email protected]>
@jdmpapin
Copy link
Contributor

Great, thank you!

Can I bother you for one last thing? The last two lines of the test aren't indented as far as the rest of it. Judging based on the rest of the file, it seems that the bulk of the test has been indented an extra level for some reason. Would you mind unindenting it in a separate commit?

Signed-off-by: Eric Yang <[email protected]>
@EricYangIBM
Copy link
Contributor Author

Done

@jdmpapin
Copy link
Contributor

Jenkins build all

@jdmpapin
Copy link
Contributor

Jenkins build win

@jdmpapin jdmpapin merged commit c862fbb into eclipse:master May 16, 2022
@EricYangIBM EricYangIBM deleted the set_limit_test branch May 17, 2022 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants