diff mbox

Test: Replace test relying pselect6(2) man page error

Message ID 1494531417-9929-1-git-send-email-francis.deslauriers@efficios.com
State Accepted, archived
Delegated to: Jérémie Galarneau
Headers show

Commit Message

Francis Deslauriers May 11, 2017, 7:36 p.m. UTC
The `pselect_fd_too_big` test is checking for the case where the `nfds`
is larger than the number of open files allowed for this process
(RLIMIT_NOFILE). According to the man page, if `nfds` > RLIMIT_NOFILE is
evaluate to true the pselect6 syscall should return EINVAL.

In fact, the Linux implementation of the pselect6 syscall[1] does not
compare the `nfds` and RLIMIT_NOFILE but rather caps `nfds` to the
highest numbered fd of the current process.

It was observed elsewhere that there is a discrepancy between the manual
page and the implementation [2].

As a solution, replace the current testcase with one that checks the
behaviour of the syscall when passed an invalid FD.

[1]:https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/select.c#n619
[2]:https://patchwork.kernel.org/patch/9345805/

Signed-off-by: Francis Deslauriers <francis.deslauriers at efficios.com>
Signed-off-by: Julien Desfossez <jdesfossez at efficios.com>
---
 tests/regression/kernel/select_poll_epoll.c        | 39 +++++++++++++---------
 tests/regression/kernel/test_select_poll_epoll     |  6 ++--
 .../kernel/validate_select_poll_epoll.py           | 13 ++++----
 3 files changed, 33 insertions(+), 25 deletions(-)

Comments

Francis Deslauriers May 11, 2017, 7:40 p.m. UTC | #1
Sorry for the missing [PATCH lttng-tools]

2017-05-11 15:36 GMT-04:00 Francis Deslauriers <
francis.deslauriers at efficios.com>:

> The `pselect_fd_too_big` test is checking for the case where the `nfds`
> is larger than the number of open files allowed for this process
> (RLIMIT_NOFILE). According to the man page, if `nfds` > RLIMIT_NOFILE is
> evaluate to true the pselect6 syscall should return EINVAL.
>
> In fact, the Linux implementation of the pselect6 syscall[1] does not
> compare the `nfds` and RLIMIT_NOFILE but rather caps `nfds` to the
> highest numbered fd of the current process.
>
> It was observed elsewhere that there is a discrepancy between the manual
> page and the implementation [2].
>
> As a solution, replace the current testcase with one that checks the
> behaviour of the syscall when passed an invalid FD.
>
> [1]:https://git.kernel.org/pub/scm/linux/kernel/git/torvalds
> /linux.git/tree/fs/select.c#n619
> [2]:https://patchwork.kernel.org/patch/9345805/
>
> Signed-off-by: Francis Deslauriers <francis.deslauriers at efficios.com>
> Signed-off-by: Julien Desfossez <jdesfossez at efficios.com>
> ---
>  tests/regression/kernel/select_poll_epoll.c        | 39
> +++++++++++++---------
>  tests/regression/kernel/test_select_poll_epoll     |  6 ++--
>  .../kernel/validate_select_poll_epoll.py           | 13 ++++----
>  3 files changed, 33 insertions(+), 25 deletions(-)
>
> diff --git a/tests/regression/kernel/select_poll_epoll.c
> b/tests/regression/kernel/select_poll_epoll.c
> index 4b703b3..1d767b0 100644
> --- a/tests/regression/kernel/select_poll_epoll.c
> +++ b/tests/regression/kernel/select_poll_epoll.c
> @@ -437,30 +437,38 @@ void ppoll_fds_ulong_max(void)
>  }
>
>  /*
> - * Select is limited to 1024 FDs, should output a pselect event
> - * with 0 FDs.
> + * Pass a invalid file descriptor to pselect6(). The syscall should return
> + * -EBADF. The recorded event should contain a ret=-EBADF (-9).
>   */
> -void pselect_fd_too_big(void)
> +void pselect_invalid_fd(void)
>  {
> -       long rfds[2048 / (sizeof(long) * CHAR_BIT)] = { 0 };
> +       fd_set rfds;
>         int ret;
> -       int fd2;
> +       int fd;
>         char buf[BUF_SIZE];
>
>         /*
> -        * Test if nfds > 1024.
> -        * Make sure ulimit is set correctly (ulimit -n 2048).
> +        * Open a file, close it and use the closed FD in the pselect6 call
>          */
> -       fd2 = dup2(wait_fd, 2047);
> -       if (fd2 != 2047) {
> -               perror("dup2");
> -               return;
> +
> +       fd = open("/dev/null", O_RDONLY);
> +       if (fd == -1) {
> +               perror("open");
> +               goto error;
>         }
>
> -       FD_SET(fd2, (fd_set *) &rfds);
> -       ret = syscall(SYS_pselect6, fd2 + 1, &rfds, NULL, NULL, NULL,
> NULL);
> +       ret = close(fd);
>
>         if (ret == -1) {
> +               perror("close");
> +               goto error;
> +       }
> +
> +       FD_ZERO(&rfds);
> +       FD_SET(fd, &rfds);
> +
> +       ret = syscall(SYS_pselect6, fd + 1, &rfds, NULL, NULL, NULL, NULL);
> +       if (ret == -1) {
>                 perror("# pselect()");
>         } else if (ret) {
>                 printf("# [pselect] data available\n");
> @@ -471,7 +479,8 @@ void pselect_fd_too_big(void)
>         } else {
>                 printf("# [pselect] timeout\n");
>         }
> -
> +error:
> +       return;
>  }
>
>  /*
> @@ -892,7 +901,7 @@ int main(int argc, const char **argv)
>                 run_working_cases();
>                 break;
>         case 3:
> -               pselect_fd_too_big();
> +               pselect_invalid_fd();
>                 break;
>         case 4:
>                 test_ppoll_big();
> diff --git a/tests/regression/kernel/test_select_poll_epoll
> b/tests/regression/kernel/test_select_poll_epoll
> index e01866f..ec034e6 100755
> --- a/tests/regression/kernel/test_select_poll_epoll
> +++ b/tests/regression/kernel/test_select_poll_epoll
> @@ -126,13 +126,13 @@ function test_timeout_cases()
>         rm -rf $TRACE_PATH
>  }
>
> -function test_big_pselect()
> +function test_pselect_invalid_fd()
>  {
>         TRACE_PATH=$(mktemp -d)
>         SESSION_NAME="syscall_payload"
>         SYSCALL_LIST="pselect6"
>
> -       diag "pselect with a FD > 1023"
> +       diag "pselect with invalid FD"
>
>         create_lttng_session_ok $SESSION_NAME $TRACE_PATH
>
> @@ -384,7 +384,7 @@ skip $isroot "Root access is needed. Skipping all
> tests." $NUM_TESTS ||
>
>         test_working_cases
>         test_timeout_cases
> -       test_big_pselect
> +       test_pselect_invalid_fd
>         test_big_ppoll
>         test_ppoll_overflow
>         test_pselect_invalid_ptr
> diff --git a/tests/regression/kernel/validate_select_poll_epoll.py
> b/tests/regression/kernel/validate_select_poll_epoll.py
> index f4946e7..613cec3 100755
> --- a/tests/regression/kernel/validate_select_poll_epoll.py
> +++ b/tests/regression/kernel/validate_select_poll_epoll.py
> @@ -450,8 +450,8 @@ class Test2(TraceParser):
>  class Test3(TraceParser):
>      def __init__(self, trace, pid):
>          super().__init__(trace, pid)
> -        self.expect["select_too_big_in"] = 0
> -        self.expect["select_too_big_out"] = 0
> +        self.expect["select_invalid_fd_in"] = 0
> +        self.expect["select_invalid_fd_out"] = 0
>
>      def select_entry(self, event):
>          timestamp = event.timestamp
> @@ -466,9 +466,8 @@ class Test3(TraceParser):
>          _exceptfds_length = event["_exceptfds_length"]
>          exceptfds = event["exceptfds"]
>
> -        # make sure an invalid value still produces a valid event
> -        if n == 2048 and overflow == 0 and _readfds_length == 0:
> -            self.expect["select_too_big_in"] = 1
> +        if n > 0 and overflow == 0:
> +            self.expect["select_invalid_fd_in"] = 1
>
>      def select_exit(self, event):
>          timestamp = event.timestamp
> @@ -483,9 +482,9 @@ class Test3(TraceParser):
>          _exceptfds_length = event["_exceptfds_length"]
>          exceptfds = event["exceptfds"]
>
> -        # make sure an invalid value still produces a valid event
> +        # make sure the event has a ret field equal to -EBADF
>          if ret == -9 and overflow == 0 and _readfds_length == 0:
> -            self.expect["select_too_big_out"] = 1
> +            self.expect["select_invalid_fd_out"] = 1
>
>
>  class Test4(TraceParser):
> --
> 2.7.4
>
>
diff mbox

Patch

diff --git a/tests/regression/kernel/select_poll_epoll.c b/tests/regression/kernel/select_poll_epoll.c
index 4b703b3..1d767b0 100644
--- a/tests/regression/kernel/select_poll_epoll.c
+++ b/tests/regression/kernel/select_poll_epoll.c
@@ -437,30 +437,38 @@  void ppoll_fds_ulong_max(void)
 }
 
 /*
- * Select is limited to 1024 FDs, should output a pselect event
- * with 0 FDs.
+ * Pass a invalid file descriptor to pselect6(). The syscall should return
+ * -EBADF. The recorded event should contain a ret=-EBADF (-9).
  */
-void pselect_fd_too_big(void)
+void pselect_invalid_fd(void)
 {
-	long rfds[2048 / (sizeof(long) * CHAR_BIT)] = { 0 };
+	fd_set rfds;
 	int ret;
-	int fd2;
+	int fd;
 	char buf[BUF_SIZE];
 
 	/*
-	 * Test if nfds > 1024.
-	 * Make sure ulimit is set correctly (ulimit -n 2048).
+	 * Open a file, close it and use the closed FD in the pselect6 call
 	 */
-	fd2 = dup2(wait_fd, 2047);
-	if (fd2 != 2047) {
-		perror("dup2");
-		return;
+
+	fd = open("/dev/null", O_RDONLY);
+	if (fd == -1) {
+		perror("open");
+		goto error;
 	}
 
-	FD_SET(fd2, (fd_set *) &rfds);
-	ret = syscall(SYS_pselect6, fd2 + 1, &rfds, NULL, NULL, NULL, NULL);
+	ret = close(fd);
 
 	if (ret == -1) {
+		perror("close");
+		goto error;
+	}
+
+	FD_ZERO(&rfds);
+	FD_SET(fd, &rfds);
+
+	ret = syscall(SYS_pselect6, fd + 1, &rfds, NULL, NULL, NULL, NULL);
+	if (ret == -1) {
 		perror("# pselect()");
 	} else if (ret) {
 		printf("# [pselect] data available\n");
@@ -471,7 +479,8 @@  void pselect_fd_too_big(void)
 	} else {
 		printf("# [pselect] timeout\n");
 	}
-
+error:
+	return;
 }
 
 /*
@@ -892,7 +901,7 @@  int main(int argc, const char **argv)
 		run_working_cases();
 		break;
 	case 3:
-		pselect_fd_too_big();
+		pselect_invalid_fd();
 		break;
 	case 4:
 		test_ppoll_big();
diff --git a/tests/regression/kernel/test_select_poll_epoll b/tests/regression/kernel/test_select_poll_epoll
index e01866f..ec034e6 100755
--- a/tests/regression/kernel/test_select_poll_epoll
+++ b/tests/regression/kernel/test_select_poll_epoll
@@ -126,13 +126,13 @@  function test_timeout_cases()
 	rm -rf $TRACE_PATH
 }
 
-function test_big_pselect()
+function test_pselect_invalid_fd()
 {
 	TRACE_PATH=$(mktemp -d)
 	SESSION_NAME="syscall_payload"
 	SYSCALL_LIST="pselect6"
 
-	diag "pselect with a FD > 1023"
+	diag "pselect with invalid FD"
 
 	create_lttng_session_ok $SESSION_NAME $TRACE_PATH
 
@@ -384,7 +384,7 @@  skip $isroot "Root access is needed. Skipping all tests." $NUM_TESTS ||
 
 	test_working_cases
 	test_timeout_cases
-	test_big_pselect
+	test_pselect_invalid_fd
 	test_big_ppoll
 	test_ppoll_overflow
 	test_pselect_invalid_ptr
diff --git a/tests/regression/kernel/validate_select_poll_epoll.py b/tests/regression/kernel/validate_select_poll_epoll.py
index f4946e7..613cec3 100755
--- a/tests/regression/kernel/validate_select_poll_epoll.py
+++ b/tests/regression/kernel/validate_select_poll_epoll.py
@@ -450,8 +450,8 @@  class Test2(TraceParser):
 class Test3(TraceParser):
     def __init__(self, trace, pid):
         super().__init__(trace, pid)
-        self.expect["select_too_big_in"] = 0
-        self.expect["select_too_big_out"] = 0
+        self.expect["select_invalid_fd_in"] = 0
+        self.expect["select_invalid_fd_out"] = 0
 
     def select_entry(self, event):
         timestamp = event.timestamp
@@ -466,9 +466,8 @@  class Test3(TraceParser):
         _exceptfds_length = event["_exceptfds_length"]
         exceptfds = event["exceptfds"]
 
-        # make sure an invalid value still produces a valid event
-        if n == 2048 and overflow == 0 and _readfds_length == 0:
-            self.expect["select_too_big_in"] = 1
+        if n > 0 and overflow == 0:
+            self.expect["select_invalid_fd_in"] = 1
 
     def select_exit(self, event):
         timestamp = event.timestamp
@@ -483,9 +482,9 @@  class Test3(TraceParser):
         _exceptfds_length = event["_exceptfds_length"]
         exceptfds = event["exceptfds"]
 
-        # make sure an invalid value still produces a valid event
+        # make sure the event has a ret field equal to -EBADF
         if ret == -9 and overflow == 0 and _readfds_length == 0:
-            self.expect["select_too_big_out"] = 1
+            self.expect["select_invalid_fd_out"] = 1
 
 
 class Test4(TraceParser):