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

pkg/sentry/syscalls/linux/sys_poll.go

@@ -82,7 +82,7 @@ func doPoll(t *kernel.Task, pfdAddr usermem.Addr, nfds uint, timeout time.Durati
 }
 
 func doSelect(t *kernel.Task, nfds int, readFDs, writeFDs, exceptFDs usermem.Addr, timeout time.Duration) (uintptr, error) {
-	if nfds < 0 || uint64(nfds) > t.ThreadGroup().Limits().GetCapped(limits.NumberOfFiles, fileCap) {
+	if nfds < 0 || nfds > fileCap {
 		return 0, syserror.EINVAL
 	}
 
@@ -90,6 +90,7 @@ func doSelect(t *kernel.Task, nfds int, readFDs, writeFDs, exceptFDs usermem.Add
 	//
 	// N.B. This only works on little-endian architectures.
 	byteCount := (nfds + 7) / 8
+
 	bitsInLastPartialByte := uint(nfds % 8)
 	r := make([]byte, byteCount)
 	w := make([]byte, byteCount)

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",

test/syscalls/linux/mlock.cc

@@ -12,18 +12,19 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#include <errno.h>
-#include <string.h>
 #include <sys/mman.h>
 #include <sys/resource.h>
 #include <sys/syscall.h>
 #include <unistd.h>
+#include <cerrno>
+#include <cstring>
 
 #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<Cleanup> 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()));

test/syscalls/linux/select.cc

@@ -12,14 +12,23 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#include <limits.h>
-#include <signal.h>
+#include <fcntl.h>
+#include <sys/resource.h>
 #include <sys/select.h>
+#include <sys/time.h>
+#include <climits>
+#include <csignal>
+#include <cstdio>
 
 #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

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",
+    ],
+)

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 <sys/resource.h>
+#include <cerrno>
+
+#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<Cleanup> 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

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 <sys/resource.h>
+#include <sys/time.h>
+
+#include "test/util/cleanup.h"
+#include "test/util/posix_error.h"
+#include "test/util/test_util.h"
+
+namespace gvisor {
+namespace testing {
+
+PosixErrorOr<Cleanup> ScopedSetSoftRlimit(int resource, rlim_t newval);
+
+}  // namespace testing
+}  // namespace gvisor
+#endif  // GVISOR_TEST_UTIL_RLIMIT_UTIL_H_