Message ID | 20171225114806.GB4341@altlinux.org |
---|---|
State | New |
Headers | show |
Series | tst-ttyname: skip the test when /dev/ptmx is not available | expand |
* Dmitry V. Levin: > * sysdeps/unix/sysv/linux/tst-ttyname.c (do_in_chroot_1): Skip the > test instead of failing in case of an error returned by posix_openpt. I think the test failure is real in this case. I wouldn't it be?
On Mon, Dec 25, 2017 at 07:19:44PM +0100, Florian Weimer wrote: > * Dmitry V. Levin: > > > * sysdeps/unix/sysv/linux/tst-ttyname.c (do_in_chroot_1): Skip the > > test instead of failing in case of an error returned by posix_openpt. > > I think the test failure is real in this case. I wouldn't it be? No, /dev/ptmx is intentionally missing in the environment where this test failed. Do you suggest an explicit check for ENOENT?
On Mon, Dec 25, 2017 at 11:12 AM, Dmitry V. Levin <ldv@altlinux.org> wrote: > On Mon, Dec 25, 2017 at 07:19:44PM +0100, Florian Weimer wrote: >> * Dmitry V. Levin: >> >> > * sysdeps/unix/sysv/linux/tst-ttyname.c (do_in_chroot_1): Skip the >> > test instead of failing in case of an error returned by posix_openpt. >> >> I think the test failure is real in this case. I wouldn't it be? > > No, /dev/ptmx is intentionally missing in the environment where this test > failed. Why? > Do you suggest an explicit check for ENOENT? If this is absolutely necessary for some reason, then it should be as narrow as possible, so yes, that would be my suggestion. zw
On Mon, Dec 25, 2017 at 12:09:56PM -0800, Zack Weinberg wrote: > On Mon, Dec 25, 2017 at 11:12 AM, Dmitry V. Levin <ldv@altlinux.org> wrote: > > On Mon, Dec 25, 2017 at 07:19:44PM +0100, Florian Weimer wrote: > >> * Dmitry V. Levin: > >> > >> > * sysdeps/unix/sysv/linux/tst-ttyname.c (do_in_chroot_1): Skip the > >> > test instead of failing in case of an error returned by posix_openpt. > >> > >> I think the test failure is real in this case. I wouldn't it be? > > > > No, /dev/ptmx is intentionally missing in the environment where this test > > failed. > > Why? It's a restricted environment. > > Do you suggest an explicit check for ENOENT? > > If this is absolutely necessary for some reason, then it should be as > narrow as possible, so yes, that would be my suggestion. OK
On Mon, Dec 25, 2017 at 1:00 PM, Dmitry V. Levin <ldv@altlinux.org> wrote: > On Mon, Dec 25, 2017 at 12:09:56PM -0800, Zack Weinberg wrote: >> On Mon, Dec 25, 2017 at 11:12 AM, Dmitry V. Levin <ldv@altlinux.org> wrote: >> > On Mon, Dec 25, 2017 at 07:19:44PM +0100, Florian Weimer wrote: >> >> * Dmitry V. Levin: >> >> >> >> > * sysdeps/unix/sysv/linux/tst-ttyname.c (do_in_chroot_1): Skip the >> >> > test instead of failing in case of an error returned by posix_openpt. >> >> >> >> I think the test failure is real in this case. I wouldn't it be? >> > >> > No, /dev/ptmx is intentionally missing in the environment where this test >> > failed. >> >> Why? > > It's a restricted environment. Why is lack of access to pseudoterminals a useful restriction to apply? What does it defend against? zw
On Mon, Dec 25, 2017 at 01:18:56PM -0800, Zack Weinberg wrote: > On Mon, Dec 25, 2017 at 1:00 PM, Dmitry V. Levin <ldv@altlinux.org> wrote: > > On Mon, Dec 25, 2017 at 12:09:56PM -0800, Zack Weinberg wrote: > >> On Mon, Dec 25, 2017 at 11:12 AM, Dmitry V. Levin <ldv@altlinux.org> wrote: > >> > On Mon, Dec 25, 2017 at 07:19:44PM +0100, Florian Weimer wrote: > >> >> * Dmitry V. Levin: > >> >> > >> >> > * sysdeps/unix/sysv/linux/tst-ttyname.c (do_in_chroot_1): Skip the > >> >> > test instead of failing in case of an error returned by posix_openpt. > >> >> > >> >> I think the test failure is real in this case. I wouldn't it be? > >> > > >> > No, /dev/ptmx is intentionally missing in the environment where this test > >> > failed. > >> > >> Why? > > > > It's a restricted environment. > > Why is lack of access to pseudoterminals a useful restriction to > apply? What does it defend against? The weakest point of that restricted environment is linux kernel, so unused functionality is disabled to narrow the attack surface.
* Dmitry V. Levin: > On Mon, Dec 25, 2017 at 12:09:56PM -0800, Zack Weinberg wrote: >> On Mon, Dec 25, 2017 at 11:12 AM, Dmitry V. Levin <ldv@altlinux.org> wrote: >> > On Mon, Dec 25, 2017 at 07:19:44PM +0100, Florian Weimer wrote: >> >> * Dmitry V. Levin: >> >> >> >> > * sysdeps/unix/sysv/linux/tst-ttyname.c (do_in_chroot_1): Skip the >> >> > test instead of failing in case of an error returned by posix_openpt. >> >> >> >> I think the test failure is real in this case. I wouldn't it be? >> > >> > No, /dev/ptmx is intentionally missing in the environment where this test >> > failed. >> >> Why? > > It's a restricted environment. I don't think the glibc test suite is supposed to pass in such an environment. If you don't provide /dev/null, /sys, or /proc to the tests, some of them will fail as well. I still think that the current test accurately reflects the inadequacy of your test environment.
On Tue, Dec 26, 2017 at 02:07:45PM +0100, Florian Weimer wrote: > * Dmitry V. Levin: > > On Mon, Dec 25, 2017 at 12:09:56PM -0800, Zack Weinberg wrote: > >> On Mon, Dec 25, 2017 at 11:12 AM, Dmitry V. Levin <ldv@altlinux.org> wrote: > >> > On Mon, Dec 25, 2017 at 07:19:44PM +0100, Florian Weimer wrote: > >> >> * Dmitry V. Levin: > >> >> > >> >> > * sysdeps/unix/sysv/linux/tst-ttyname.c (do_in_chroot_1): Skip the > >> >> > test instead of failing in case of an error returned by posix_openpt. > >> >> > >> >> I think the test failure is real in this case. I wouldn't it be? > >> > > >> > No, /dev/ptmx is intentionally missing in the environment where this test > >> > failed. > >> > >> Why? > > > > It's a restricted environment. > > I don't think the glibc test suite is supposed to pass in such an > environment. It used to work perfectly for at least 15 years, and it still works when tst-ttyname is fixed. > If you don't provide /dev/null, /sys, or /proc to the > tests, some of them will fail as well. This is irrelevant to /dev/ptmx availability. > I still think that the current > test accurately reflects the inadequacy of your test environment. I do not agree. btw, as support_become_root is also unavailable in restricted environments, an alternative fix for tst-ttyname is to skip the test when support_become_root fails.
On Tue, Dec 26, 2017 at 04:53:30PM +0300, Dmitry V. Levin wrote: > On Tue, Dec 26, 2017 at 02:07:45PM +0100, Florian Weimer wrote: > > * Dmitry V. Levin: > > > On Mon, Dec 25, 2017 at 12:09:56PM -0800, Zack Weinberg wrote: > > >> On Mon, Dec 25, 2017 at 11:12 AM, Dmitry V. Levin <ldv@altlinux.org> wrote: > > >> > On Mon, Dec 25, 2017 at 07:19:44PM +0100, Florian Weimer wrote: > > >> >> * Dmitry V. Levin: > > >> >> > > >> >> > * sysdeps/unix/sysv/linux/tst-ttyname.c (do_in_chroot_1): Skip the > > >> >> > test instead of failing in case of an error returned by posix_openpt. > > >> >> > > >> >> I think the test failure is real in this case. I wouldn't it be? > > >> > > > >> > No, /dev/ptmx is intentionally missing in the environment where this test > > >> > failed. > > >> > > >> Why? > > > > > > It's a restricted environment. > > > > I don't think the glibc test suite is supposed to pass in such an > > environment. > > It used to work perfectly for at least 15 years, and it still works > when tst-ttyname is fixed. As tst-ttyname was backported to 2.26 stable branch recently, it introduced this test suite regression there, too, so a fix to tst-ttyname should also be backported. [...] > btw, as support_become_root is also unavailable in restricted > environments, an alternative fix for tst-ttyname is to skip the test > when support_become_root fails. If /dev/ptmx is available but support_become_root is unable to become root, tst-ttyname correctly recognizes this setup as unsupported. It would be quite logical for tst-ttyname to return EXIT_UNSUPPORTED as soon as support_become_root failed.
* Dmitry V. Levin: >> I don't think the glibc test suite is supposed to pass in such an >> environment. > > It used to work perfectly for at least 15 years, and it still works > when tst-ttyname is fixed. This just reflects that we did not have sufficient test coverage for the PTY code. > btw, as support_become_root is also unavailable in restricted > environments, an alternative fix for tst-ttyname is to skip the test > when support_become_root fails. But that's unrelated. And in some environments, support_become_root can fail, but the process was initially sufficiently privileged to run the test.
On Fri, Dec 29, 2017 at 02:55:17PM +0100, Florian Weimer wrote: > * Dmitry V. Levin: > > >> I don't think the glibc test suite is supposed to pass in such an > >> environment. > > > > It used to work perfectly for at least 15 years, and it still works > > when tst-ttyname is fixed. > > This just reflects that we did not have sufficient test coverage for > the PTY code. No doubts about that, but the kernel side of pty interface does not have to be implemented, so we cannot allow the test to fail just because the kernel does not provide it. > > btw, as support_become_root is also unavailable in restricted > > environments, an alternative fix for tst-ttyname is to skip the test > > when support_become_root fails. > > But that's unrelated. And in some environments, support_become_root > can fail, but the process was initially sufficiently privileged to run > the test. I don't think such environments exist in practice but this is theoretically possible.
On Wed, 27 Dec 2017, Dmitry V. Levin wrote: > > > > It's a restricted environment. > > > > > > I don't think the glibc test suite is supposed to pass in such an > > > environment. > > > > It used to work perfectly for at least 15 years, and it still works > > when tst-ttyname is fixed. > > As tst-ttyname was backported to 2.26 stable branch recently, it > introduced this test suite regression there, too, so a fix to tst-ttyname > should also be backported. I'm more concerned that we still have the failure on Ubuntu 16.04 noted in <https://sourceware.org/ml/libc-alpha/2017-11/msg00832.html>, than about failures in a particular restricted environment. Generally, if there are known unresolved issues around a patch on master, that's evidence it would be premature to backport it to a stable branch. (Note: I haven't actually verified if the failure on Ubuntu 16.04 is now present on the release branch.)
On Sun, 31 Dec 2017 20:30:16 -0500, Joseph Myers wrote: > On Wed, 27 Dec 2017, Dmitry V. Levin wrote: > > > > > > It's a restricted environment. > > > > > > > > I don't think the glibc test suite is supposed to pass in such an > > > > environment. > > > > > > It used to work perfectly for at least 15 years, and it still works > > > when tst-ttyname is fixed. > > > > As tst-ttyname was backported to 2.26 stable branch recently, it > > introduced this test suite regression there, too, so a fix to tst-ttyname > > should also be backported. > > I'm more concerned that we still have the failure on Ubuntu 16.04 noted in > <https://sourceware.org/ml/libc-alpha/2017-11/msg00832.html>, than about > failures in a particular restricted environment. Generally, if there are > known unresolved issues around a patch on master, that's evidence it would > be premature to backport it to a stable branch. (Note: I haven't actually > verified if the failure on Ubuntu 16.04 is now present on the release > branch.) Yikes! I didn't notice that reply. I've just downloaded the Ubuntu 16.04.3 ISO and will look in to that in the next few days.
diff --git a/sysdeps/unix/sysv/linux/tst-ttyname.c b/sysdeps/unix/sysv/linux/tst-ttyname.c index 0fdf1a8..15f6c4c 100644 --- a/sysdeps/unix/sysv/linux/tst-ttyname.c +++ b/sysdeps/unix/sysv/linux/tst-ttyname.c @@ -253,7 +253,9 @@ do_in_chroot_1 (int (*cb)(const char *, int)) /* Open the PTS that we'll be testing on. */ int master; char *slavename; - VERIFY ((master = posix_openpt (O_RDWR|O_NOCTTY|O_NONBLOCK)) >= 0); + master = posix_openpt (O_RDWR|O_NOCTTY|O_NONBLOCK); + if (master < 0) + FAIL_UNSUPPORTED ("posix_openpt: %m"); VERIFY ((slavename = ptsname (master))); VERIFY (unlockpt (master) == 0); if (strncmp (slavename, "/dev/pts/", 9) != 0)