public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] tst-ttyname: skip the test when /dev/ptmx is not available
@ 2017-12-25 11:48 Dmitry V. Levin
  2017-12-25 18:19 ` Florian Weimer
  0 siblings, 1 reply; 18+ messages in thread
From: Dmitry V. Levin @ 2017-12-25 11:48 UTC (permalink / raw)
  To: libc-alpha

* 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.
---
 ChangeLog                             | 5 +++++
 sysdeps/unix/sysv/linux/tst-ttyname.c | 4 +++-
 2 files changed, 8 insertions(+), 1 deletion(-)

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)
-- 
ldv

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] tst-ttyname: skip the test when /dev/ptmx is not available
  2017-12-25 11:48 [PATCH] tst-ttyname: skip the test when /dev/ptmx is not available Dmitry V. Levin
@ 2017-12-25 18:19 ` Florian Weimer
  2017-12-25 19:13   ` Dmitry V. Levin
  0 siblings, 1 reply; 18+ messages in thread
From: Florian Weimer @ 2017-12-25 18:19 UTC (permalink / raw)
  To: libc-alpha

* 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?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] tst-ttyname: skip the test when /dev/ptmx is not available
  2017-12-25 18:19 ` Florian Weimer
@ 2017-12-25 19:13   ` Dmitry V. Levin
  2017-12-25 20:10     ` Zack Weinberg
  0 siblings, 1 reply; 18+ messages in thread
From: Dmitry V. Levin @ 2017-12-25 19:13 UTC (permalink / raw)
  To: libc-alpha

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

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?


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] tst-ttyname: skip the test when /dev/ptmx is not available
  2017-12-25 19:13   ` Dmitry V. Levin
@ 2017-12-25 20:10     ` Zack Weinberg
  2017-12-25 21:01       ` Dmitry V. Levin
  0 siblings, 1 reply; 18+ messages in thread
From: Zack Weinberg @ 2017-12-25 20:10 UTC (permalink / raw)
  To: GNU C Library

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] tst-ttyname: skip the test when /dev/ptmx is not available
  2017-12-25 20:10     ` Zack Weinberg
@ 2017-12-25 21:01       ` Dmitry V. Levin
  2017-12-25 21:19         ` Zack Weinberg
  2017-12-26 13:07         ` Florian Weimer
  0 siblings, 2 replies; 18+ messages in thread
From: Dmitry V. Levin @ 2017-12-25 21:01 UTC (permalink / raw)
  To: libc-alpha

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

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


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] tst-ttyname: skip the test when /dev/ptmx is not available
  2017-12-25 21:01       ` Dmitry V. Levin
@ 2017-12-25 21:19         ` Zack Weinberg
  2017-12-25 21:39           ` Dmitry V. Levin
  2017-12-26 13:07         ` Florian Weimer
  1 sibling, 1 reply; 18+ messages in thread
From: Zack Weinberg @ 2017-12-25 21:19 UTC (permalink / raw)
  To: GNU C Library

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] tst-ttyname: skip the test when /dev/ptmx is not available
  2017-12-25 21:19         ` Zack Weinberg
@ 2017-12-25 21:39           ` Dmitry V. Levin
  0 siblings, 0 replies; 18+ messages in thread
From: Dmitry V. Levin @ 2017-12-25 21:39 UTC (permalink / raw)
  To: libc-alpha

[-- Attachment #1: Type: text/plain, Size: 1070 bytes --]

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.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] tst-ttyname: skip the test when /dev/ptmx is not available
  2017-12-25 21:01       ` Dmitry V. Levin
  2017-12-25 21:19         ` Zack Weinberg
@ 2017-12-26 13:07         ` Florian Weimer
  2017-12-26 13:53           ` Dmitry V. Levin
  2017-12-30 23:09           ` Luke Shumaker
  1 sibling, 2 replies; 18+ messages in thread
From: Florian Weimer @ 2017-12-26 13:07 UTC (permalink / raw)
  To: libc-alpha

* 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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] tst-ttyname: skip the test when /dev/ptmx is not available
  2017-12-26 13:07         ` Florian Weimer
@ 2017-12-26 13:53           ` Dmitry V. Levin
  2017-12-26 23:07             ` Dmitry V. Levin
  2017-12-29 14:20             ` Florian Weimer
  2017-12-30 23:09           ` Luke Shumaker
  1 sibling, 2 replies; 18+ messages in thread
From: Dmitry V. Levin @ 2017-12-26 13:53 UTC (permalink / raw)
  To: libc-alpha

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.


-- 
ldv

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3] tst-ttyname: skip the test if failed to become root
@ 2017-12-26 14:10 Dmitry V. Levin
  0 siblings, 0 replies; 18+ messages in thread
From: Dmitry V. Levin @ 2017-12-26 14:10 UTC (permalink / raw)
  To: libc-alpha

* sysdeps/unix/sysv/linux/tst-ttyname.c (do_test): Skip the test
if support_become_root failed.
---
 ChangeLog                             | 5 +++++
 sysdeps/unix/sysv/linux/tst-ttyname.c | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/sysdeps/unix/sysv/linux/tst-ttyname.c b/sysdeps/unix/sysv/linux/tst-ttyname.c
index 0fdf1a8..3943403 100644
--- a/sysdeps/unix/sysv/linux/tst-ttyname.c
+++ b/sysdeps/unix/sysv/linux/tst-ttyname.c
@@ -554,7 +554,8 @@ run_chroot_tests (const char *slavename, int slave)
 static int
 do_test (void)
 {
-  support_become_root ();
+  if (!support_become_root ())
+    return EXIT_UNSUPPORTED;
 
   int ret1 = do_in_chroot_1 (run_chroot_tests);
   if (ret1 == EXIT_UNSUPPORTED)
-- 
ldv

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] tst-ttyname: skip the test when /dev/ptmx is not available
  2017-12-26 13:53           ` Dmitry V. Levin
@ 2017-12-26 23:07             ` Dmitry V. Levin
  2018-01-01  1:30               ` Joseph Myers
  2017-12-29 14:20             ` Florian Weimer
  1 sibling, 1 reply; 18+ messages in thread
From: Dmitry V. Levin @ 2017-12-26 23:07 UTC (permalink / raw)
  To: libc-alpha

[-- Attachment #1: Type: text/plain, Size: 1672 bytes --]

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.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] tst-ttyname: skip the test when /dev/ptmx is not available
  2017-12-26 13:53           ` Dmitry V. Levin
  2017-12-26 23:07             ` Dmitry V. Levin
@ 2017-12-29 14:20             ` Florian Weimer
  2017-12-29 14:49               ` Dmitry V. Levin
  2018-01-02  5:19               ` [PATCH v3] tst-ttyname: skip the test if failed to become root Luke Shumaker
  1 sibling, 2 replies; 18+ messages in thread
From: Florian Weimer @ 2017-12-29 14:20 UTC (permalink / raw)
  To: libc-alpha

* 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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] tst-ttyname: skip the test when /dev/ptmx is not available
  2017-12-29 14:20             ` Florian Weimer
@ 2017-12-29 14:49               ` Dmitry V. Levin
  2018-01-02  5:19               ` [PATCH v3] tst-ttyname: skip the test if failed to become root Luke Shumaker
  1 sibling, 0 replies; 18+ messages in thread
From: Dmitry V. Levin @ 2017-12-29 14:49 UTC (permalink / raw)
  To: libc-alpha

[-- Attachment #1: Type: text/plain, Size: 1010 bytes --]

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.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3] tst-ttyname: skip the test if failed to become root
  2017-12-26 13:07         ` Florian Weimer
  2017-12-26 13:53           ` Dmitry V. Levin
@ 2017-12-30 23:09           ` Luke Shumaker
  2017-12-31  0:11             ` Dmitry V. Levin
  1 sibling, 1 reply; 18+ messages in thread
From: Luke Shumaker @ 2017-12-30 23:09 UTC (permalink / raw)
  To: libc-alpha

Dmitry V. Levin wrote:
> It would be quite logical for tst-ttyname to return EXIT_UNSUPPORTED
> as soon as support_become_root failed.

Indeed; in the original version of the test, whereit called
support_become_root, it said:

      support_become_root ();
    
      if (unshare (CLONE_NEWNS) < 0)
        FAIL_UNSUPPORTED ("could not enter new mount namespace");

Where, that call to `unshare (CLONE_NEWNS)` is guaranteed to to fail
if `support_become_root ()` failed; it would have been pointless to
check for failure twice in the same spot by directly checking
support_become_root.

However, when Florian cleaned up the test, the call to
support_become_root got moved to be much earlier in the test; my above
assumtion that we would immediately be doing something that would
implicitly check the success of support_become_root became invalid.
Of course, I didn't realize that when I said "LGTM" To Florian's
patch.

If the only problem with the test is that support_become_root fails,
then we can count on the later

      if (!support_enter_mount_namespace ())
	FAIL_UNSUPPORTED ("could not enter new mount namespace");

causing us to exit with EXIT_UNSUPPORTED.  But, checking the result of
support_become_root allows us to exit earlier; saving pointless work.

On Tue, 26 Dec 2017 09:10:02 -0500,
Dmitry V. Levin wrote:
> diff --git a/sysdeps/unix/sysv/linux/tst-ttyname.c b/sysdeps/unix/sysv/linux/tst-ttyname.c
> index 0fdf1a8..3943403 100644
> --- a/sysdeps/unix/sysv/linux/tst-ttyname.c
> +++ b/sysdeps/unix/sysv/linux/tst-ttyname.c
> @@ -554,7 +554,8 @@ run_chroot_tests (const char *slavename, int slave)
>  static int
>  do_test (void)
>  {
> -  support_become_root ();
> +  if (!support_become_root ())
> +    return EXIT_UNSUPPORTED;
>  
>    int ret1 = do_in_chroot_1 (run_chroot_tests);
>    if (ret1 == EXIT_UNSUPPORTED)

So, with the above in mind, I do think this patch should be applied.

But, I do agree with Florian when he said

Florian Weimer wrote:
> I don't think the glibc test suite is supposed to pass in such an
> environment [without ptmx].  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.

So I don't nescessarily think that this should be backported to the
2.26 stable branch.

-- 
Happy hacking,
~ Luke Shumaker

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3] tst-ttyname: skip the test if failed to become root
  2017-12-30 23:09           ` Luke Shumaker
@ 2017-12-31  0:11             ` Dmitry V. Levin
  0 siblings, 0 replies; 18+ messages in thread
From: Dmitry V. Levin @ 2017-12-31  0:11 UTC (permalink / raw)
  To: libc-alpha

[-- Attachment #1: Type: text/plain, Size: 3281 bytes --]

On Sat, Dec 30, 2017 at 06:09:22PM -0500, Luke Shumaker wrote:
> Dmitry V. Levin wrote:
> > It would be quite logical for tst-ttyname to return EXIT_UNSUPPORTED
> > as soon as support_become_root failed.
> 
> Indeed; in the original version of the test, whereit called
> support_become_root, it said:
> 
>       support_become_root ();
>     
>       if (unshare (CLONE_NEWNS) < 0)
>         FAIL_UNSUPPORTED ("could not enter new mount namespace");
> 
> Where, that call to `unshare (CLONE_NEWNS)` is guaranteed to to fail
> if `support_become_root ()` failed; it would have been pointless to
> check for failure twice in the same spot by directly checking
> support_become_root.
> 
> However, when Florian cleaned up the test, the call to
> support_become_root got moved to be much earlier in the test; my above
> assumtion that we would immediately be doing something that would
> implicitly check the success of support_become_root became invalid.
> Of course, I didn't realize that when I said "LGTM" To Florian's
> patch.
> 
> If the only problem with the test is that support_become_root fails,
> then we can count on the later
> 
>       if (!support_enter_mount_namespace ())
> 	FAIL_UNSUPPORTED ("could not enter new mount namespace");
> 
> causing us to exit with EXIT_UNSUPPORTED.  But, checking the result of
> support_become_root allows us to exit earlier; saving pointless work.
> 
> On Tue, 26 Dec 2017 09:10:02 -0500,
> Dmitry V. Levin wrote:
> > diff --git a/sysdeps/unix/sysv/linux/tst-ttyname.c b/sysdeps/unix/sysv/linux/tst-ttyname.c
> > index 0fdf1a8..3943403 100644
> > --- a/sysdeps/unix/sysv/linux/tst-ttyname.c
> > +++ b/sysdeps/unix/sysv/linux/tst-ttyname.c
> > @@ -554,7 +554,8 @@ run_chroot_tests (const char *slavename, int slave)
> >  static int
> >  do_test (void)
> >  {
> > -  support_become_root ();
> > +  if (!support_become_root ())
> > +    return EXIT_UNSUPPORTED;
> >  
> >    int ret1 = do_in_chroot_1 (run_chroot_tests);
> >    if (ret1 == EXIT_UNSUPPORTED)
> 
> So, with the above in mind, I do think this patch should be applied.
> 
> But, I do agree with Florian when he said
> 
> Florian Weimer wrote:
> > I don't think the glibc test suite is supposed to pass in such an
> > environment [without ptmx].  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.
> 
> So I don't nescessarily think that this should be backported to the
> 2.26 stable branch.

If a backported test introduced a regression in the test suite, then a fix
has to be backported as well, I don't think anybody argues with that.

This is exactly the case - tst-ttyname was backported to the stable branch
and caused a regression in the test suite there.  One cannot say that
environments without ptmx are not supported by 2.26 - the fact is that
they were definitely supported at 2.26 release time.

Also, one cannot declare these environments unsupported in master branch
without reaching a consensus.  tst-ttyname was not purposefully made to
fail when ptmx is not available - it's just an omission that has to be
fixed.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] tst-ttyname: skip the test when /dev/ptmx is not available
  2017-12-26 23:07             ` Dmitry V. Levin
@ 2018-01-01  1:30               ` Joseph Myers
  2018-01-01  2:28                 ` Luke Shumaker
  0 siblings, 1 reply; 18+ messages in thread
From: Joseph Myers @ 2018-01-01  1:30 UTC (permalink / raw)
  To: Dmitry V. Levin; +Cc: libc-alpha

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.)

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] tst-ttyname: skip the test when /dev/ptmx is not available
  2018-01-01  1:30               ` Joseph Myers
@ 2018-01-01  2:28                 ` Luke Shumaker
  0 siblings, 0 replies; 18+ messages in thread
From: Luke Shumaker @ 2018-01-01  2:28 UTC (permalink / raw)
  To: Joseph Myers; +Cc: Dmitry V. Levin, libc-alpha

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.

-- 
Happy hacking,
~ Luke Shumaker

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3] tst-ttyname: skip the test if failed to become root
  2017-12-29 14:20             ` Florian Weimer
  2017-12-29 14:49               ` Dmitry V. Levin
@ 2018-01-02  5:19               ` Luke Shumaker
  1 sibling, 0 replies; 18+ messages in thread
From: Luke Shumaker @ 2018-01-02  5:19 UTC (permalink / raw)
  To: libc-alpha

On Sat, 30 Dec 2017 19:11:52 -0500,
Dmitry V. Levin wrote:
> On Sat, Dec 30, 2017 at 06:09:22PM -0500, Luke Shumaker wrote:
> > Florian Weimer wrote:
> > > I don't think the glibc test suite is supposed to pass in such an
> > > environment [without ptmx].  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.
>
> Also, one cannot declare these environments unsupported in master branch
> without reaching a consensus.

On Fri, 29 Dec 2017 08:55:17 -0500,
Florian Weimer wrote:
> * Dmitry V. Levin:
> > 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.

Obviously, I'm the outsider here; and can't authoritatively comment on
whether glibc supports environments without ptmx / UNIX 98 PTYs, but
from looking over the codebase:

 1. There is insufficient test coverate of posix_openpt and friends to
    conclude that Linux environments without ptmx were previously
    supported.

    The only other tests that call posix_openpt or other PTY-opening
    functions are

     - debug/tst-chk1.c
     - login/tst-grantpt.c
     - login/tst-ptsname.c
    
    I don't have a good idea of what tst-chk1 does, or why it doesn't
    error if posix_openpt fails; but the other 2 ignore an error
    because "maybe the system does not have SUS pseudo terminals", and
    the posix_openpt bit is just one subtest case.

    Which is to say that posix_openpt is previously untested by the
    test suite.

    But, Linux does support SUS / UNIX 98 pseudo terminals; and
    tst-ttyname lives in sysdeps/unix/sysv/linux.  Linux has no config
    option to disable ptmx.  The only variable there is whatever sets
    up /dev.

 2. The Linux implementation of getpt (a GNU extension) first tries
    posix_openpt to get a UNIX 98 PTY, but if that fails, it falls
    back to trying to get a BSD PTY.  Which suggests that glibc
    supports Linux environments without ptmx.

I find #2 fairly compelling.  That is to say, I've changed my mind,
and do support applying and backporting a patch to EXIT_UNSUPPORTED if
posix_openpt returns ENOENT.

-- 
Happy hacking,
~ Luke Shumaker

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2018-01-02  5:19 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-25 11:48 [PATCH] tst-ttyname: skip the test when /dev/ptmx is not available Dmitry V. Levin
2017-12-25 18:19 ` Florian Weimer
2017-12-25 19:13   ` Dmitry V. Levin
2017-12-25 20:10     ` Zack Weinberg
2017-12-25 21:01       ` Dmitry V. Levin
2017-12-25 21:19         ` Zack Weinberg
2017-12-25 21:39           ` Dmitry V. Levin
2017-12-26 13:07         ` Florian Weimer
2017-12-26 13:53           ` Dmitry V. Levin
2017-12-26 23:07             ` Dmitry V. Levin
2018-01-01  1:30               ` Joseph Myers
2018-01-01  2:28                 ` Luke Shumaker
2017-12-29 14:20             ` Florian Weimer
2017-12-29 14:49               ` Dmitry V. Levin
2018-01-02  5:19               ` [PATCH v3] tst-ttyname: skip the test if failed to become root Luke Shumaker
2017-12-30 23:09           ` Luke Shumaker
2017-12-31  0:11             ` Dmitry V. Levin
2017-12-26 14:10 Dmitry V. Levin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).