From 7f8de3bf92decbd745a4bc4e8aebf1ba1159ed4b Mon Sep 17 00:00:00 2001 From: Zach Koopmans Date: Thu, 10 Jan 2019 09:43:43 -0800 Subject: Fixing select call to not enforce RLIMIT_NOFILE. Removing check to RLIMIT_NOFILE in select call. Adding unit test to select suite to document behavior. Moving setrlimit class from mlock to a util file for reuse. Fixing flaky test based on comments from Jamie. PiperOrigin-RevId: 228726131 Change-Id: Ie9dbe970bbf835ba2cca6e17eec7c2ee6fadf459 --- test/syscalls/linux/BUILD | 6 +++++ test/syscalls/linux/mlock.cc | 19 +++---------- test/syscalls/linux/select.cc | 62 +++++++++++++++++++++++++++++++++++-------- test/util/BUILD | 13 +++++++++ test/util/rlimit_util.cc | 44 ++++++++++++++++++++++++++++++ test/util/rlimit_util.h | 32 ++++++++++++++++++++++ 6 files changed, 149 insertions(+), 27 deletions(-) create mode 100644 test/util/rlimit_util.cc create mode 100644 test/util/rlimit_util.h (limited to 'test') diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD index 028c686a8..a8a6e15ee 100644 --- a/test/syscalls/linux/BUILD +++ b/test/syscalls/linux/BUILD @@ -1033,6 +1033,7 @@ cc_binary( "//test/util:cleanup", "//test/util:memory_util", "//test/util:multiprocess_util", + "//test/util:rlimit_util", "//test/util:test_main", "//test/util:test_util", "@com_google_googletest//:gtest", @@ -1650,6 +1651,11 @@ cc_binary( linkstatic = 1, deps = [ ":base_poll_test", + "//test/util:file_descriptor", + "//test/util:multiprocess_util", + "//test/util:posix_error", + "//test/util:rlimit_util", + "//test/util:temp_path", "//test/util:test_main", "//test/util:test_util", "@com_google_absl//absl/time", diff --git a/test/syscalls/linux/mlock.cc b/test/syscalls/linux/mlock.cc index 1d93bff58..a492b2404 100644 --- a/test/syscalls/linux/mlock.cc +++ b/test/syscalls/linux/mlock.cc @@ -12,18 +12,19 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include -#include #include #include #include #include +#include +#include #include "gmock/gmock.h" #include "test/util/capability_util.h" #include "test/util/cleanup.h" #include "test/util/memory_util.h" #include "test/util/multiprocess_util.h" +#include "test/util/rlimit_util.h" #include "test/util/test_util.h" using ::testing::_; @@ -58,20 +59,6 @@ bool IsPageMlocked(uintptr_t addr) { return true; } -PosixErrorOr ScopedSetSoftRlimit(int resource, rlim_t newval) { - struct rlimit old_rlim; - if (getrlimit(resource, &old_rlim) != 0) { - return PosixError(errno, "getrlimit failed"); - } - struct rlimit new_rlim = old_rlim; - new_rlim.rlim_cur = newval; - if (setrlimit(resource, &new_rlim) != 0) { - return PosixError(errno, "setrlimit failed"); - } - return Cleanup([resource, old_rlim] { - TEST_PCHECK(setrlimit(resource, &old_rlim) == 0); - }); -} TEST(MlockTest, Basic) { SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(CanMlock())); diff --git a/test/syscalls/linux/select.cc b/test/syscalls/linux/select.cc index 6b6fa9217..41e6043cc 100644 --- a/test/syscalls/linux/select.cc +++ b/test/syscalls/linux/select.cc @@ -12,14 +12,23 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include -#include +#include +#include #include +#include +#include +#include +#include #include "gtest/gtest.h" #include "gtest/gtest.h" #include "absl/time/time.h" #include "test/syscalls/linux/base_poll_test.h" +#include "test/util/file_descriptor.h" +#include "test/util/multiprocess_util.h" +#include "test/util/posix_error.h" +#include "test/util/rlimit_util.h" +#include "test/util/temp_path.h" #include "test/util/test_util.h" namespace gvisor { @@ -57,15 +66,27 @@ TEST_F(SelectTest, NegativeNfds) { } TEST_F(SelectTest, ClosedFds) { - fd_set read_set; - FD_ZERO(&read_set); - int fd; - ASSERT_THAT(fd = dup(1), SyscallSucceeds()); - ASSERT_THAT(close(fd), SyscallSucceeds()); - FD_SET(fd, &read_set); - struct timeval timeout = absl::ToTimeval(absl::Milliseconds(10)); - EXPECT_THAT(select(fd + 1, &read_set, nullptr, nullptr, &timeout), - SyscallFailsWithErrno(EBADF)); + auto temp_file = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile()); + FileDescriptor fd = + ASSERT_NO_ERRNO_AND_VALUE(Open(temp_file.path(), O_RDONLY)); + + // We can't rely on a file descriptor being closed in a multi threaded + // application so fork to get a clean process. + EXPECT_THAT(InForkedProcess([&] { + int fd_num = fd.get(); + fd.reset(); + + fd_set read_set; + FD_ZERO(&read_set); + FD_SET(fd_num, &read_set); + + struct timeval timeout = + absl::ToTimeval(absl::Milliseconds(10)); + TEST_PCHECK(select(fd_num + 1, &read_set, nullptr, nullptr, + &timeout) != 0); + TEST_PCHECK(errno == EBADF); + }), + IsPosixErrorOkAndHolds(0)); } TEST_F(SelectTest, ZeroTimeout) { @@ -123,6 +144,25 @@ TEST_F(SelectTest, IgnoreBitsAboveNfds) { SyscallSucceedsWithValue(0)); } +// This test illustrates Linux's behavior of 'select' calls passing after +// setrlimit RLIMIT_NOFILE is called. In particular, versions of sshd rely on +// this behavior. +TEST_F(SelectTest, SetrlimitCallNOFILE) { + fd_set read_set; + FD_ZERO(&read_set); + timeval timeout = {}; + const FileDescriptor fd = ASSERT_NO_ERRNO_AND_VALUE( + Open(NewTempAbsPath(), O_RDONLY | O_CREAT, S_IRUSR)); + + Cleanup reset_rlimit = + ASSERT_NO_ERRNO_AND_VALUE(ScopedSetSoftRlimit(RLIMIT_NOFILE, 0)); + + FD_SET(fd.get(), &read_set); + // this call with zero timeout should return immediately + EXPECT_THAT(select(fd.get() + 1, &read_set, nullptr, nullptr, &timeout), + SyscallSucceeds()); +} + } // namespace } // namespace testing } // namespace gvisor diff --git a/test/util/BUILD b/test/util/BUILD index 10507eae4..6316fec6e 100644 --- a/test/util/BUILD +++ b/test/util/BUILD @@ -272,3 +272,16 @@ cc_library( "@com_google_googletest//:gtest", ], ) + +cc_library( + name = "rlimit_util", + testonly = 1, + srcs = ["rlimit_util.cc"], + hdrs = ["rlimit_util.h"], + deps = [ + ":cleanup", + ":logging", + ":posix_error", + ":test_util", + ], +) diff --git a/test/util/rlimit_util.cc b/test/util/rlimit_util.cc new file mode 100644 index 000000000..a9912c372 --- /dev/null +++ b/test/util/rlimit_util.cc @@ -0,0 +1,44 @@ +// Copyright 2019 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "test/util/rlimit_util.h" + +#include +#include + +#include "test/util/cleanup.h" +#include "test/util/logging.h" +#include "test/util/posix_error.h" +#include "test/util/test_util.h" + +namespace gvisor { +namespace testing { + +PosixErrorOr ScopedSetSoftRlimit(int resource, rlim_t newval) { + struct rlimit old_rlim; + if (getrlimit(resource, &old_rlim) != 0) { + return PosixError(errno, "getrlimit failed"); + } + struct rlimit new_rlim = old_rlim; + new_rlim.rlim_cur = newval; + if (setrlimit(resource, &new_rlim) != 0) { + return PosixError(errno, "setrlimit failed"); + } + return Cleanup([resource, old_rlim] { + TEST_PCHECK(setrlimit(resource, &old_rlim) == 0); + }); +} + +} // namespace testing +} // namespace gvisor diff --git a/test/util/rlimit_util.h b/test/util/rlimit_util.h new file mode 100644 index 000000000..fa5cc70dc --- /dev/null +++ b/test/util/rlimit_util.h @@ -0,0 +1,32 @@ +// Copyright 2019 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef GVISOR_TEST_UTIL_RLIMIT_UTIL_H_ +#define GVISOR_TEST_UTIL_RLIMIT_UTIL_H_ + +#include +#include + +#include "test/util/cleanup.h" +#include "test/util/posix_error.h" +#include "test/util/test_util.h" + +namespace gvisor { +namespace testing { + +PosixErrorOr ScopedSetSoftRlimit(int resource, rlim_t newval); + +} // namespace testing +} // namespace gvisor +#endif // GVISOR_TEST_UTIL_RLIMIT_UTIL_H_ -- cgit v1.2.3