Get ttyname() to work properly in containers

Bug #1669578 reported by Stéphane Graber
26
This bug affects 4 people
Affects Status Importance Assigned to Milestone
glibc (Ubuntu)
Fix Released
High
Adam Conrad
multitail (Ubuntu)
Confirmed
Undecided
Unassigned
screen (Ubuntu)
Fix Released
High
Christian Brauner
tmux (Ubuntu)
Fix Released
High
Christian Brauner

Bug Description

For the past year or so, the LXD team has been trying to resolve an issue affecting screen, tmux and a bunch of other software (including the "tty" command).

The problem comes from the fact that when attaching to a container, your terminal's pts device comes from the host and therefore can't be found in /dev/pts/.

glibc makes the assumption that it can readlink /proc/self/fd/0 and that the target path will exist. This simply isn't true as the symlink target returned by the kernel, is confusingly relative to the host's root and not the container's.

Which means that if the target happens to exist, it's actually going to be an entirely different pts device from the one that you're actually attached to.

You therefore need to do something along the lines of:
 - Resolve the symlink. If the target doesn't exist, return the symlink as the ttyname.
 - If the target does exist, check that its major and minor matches that of the symlink itself, if it doesn't, then return the symlink rather than the target.

That's the ideal approach which makes existing software keep working properly without the need for any added code. After about a year of bikeshedding, the proposed glibc upstream fix has now evolved to instead returning ENODEV in the is_pty function. This allows downstream glibc users to detect this case and then use /proc/self/fd/0 rather than the return value of ttyname() but means every software using ttyname() now needs fixing.

As we very much care about Ubuntu running properly inside LXD containers, our suggested patchset includes both the ENODEV patch as is still being considered by upstream (stuck on legal validation) AND another patch which has ttyname() return the symlink when it receives the ENODEV.

We feel this is the best way to fix the problem entirely right now. Once glibc upstream merges the ENODEV side of this and all affected software get fixed upstream to deal with it, we'll then be able to drop that patch without causing any regressions.

Tags: patch
Revision history for this message
Stéphane Graber (stgraber) wrote :

Upstream threads: https://sourceware.org/cgi-bin/search.cgi?q=ttyname+and+ttyname_r&cmd=Search%21&form=extended&m=all&ps=10&fmt=long&wm=wrd&sp=1&sy=1&wf=2221&type=&GroupBySite=no&ul=%2Fml%2Flibc-alpha%2F%25

We've been getting about a report a week about things failing due to this bug so we really need it sorted this time (most reports are for screen/tmux and tools that use them like do-release-upgrade).

Revision history for this message
Stéphane Graber (stgraber) wrote :

I'm building a patched glibc now with the two patches we'd like included in Ubuntu.
Once that's built and tested, I'll attach the debdiff.

Revision history for this message
Stéphane Graber (stgraber) wrote :

stgraber@dakara:~$ lxc exec zesty bash
root@zesty:~# tmux
lost server
root@zesty:~# tty
not a tty
root@zesty:~# echo $?
0
root@zesty:~# dpkg -i *.deb
(Reading database ... 26464 files and directories currently installed.)
Preparing to unpack libc-bin_2.24-7ubuntu3_amd64.deb ...
Unpacking libc-bin (2.24-7ubuntu3) over (2.24-7ubuntu2) ...
Preparing to unpack libc6_2.24-7ubuntu3_amd64.deb ...
Unpacking libc6:amd64 (2.24-7ubuntu3) over (2.24-7ubuntu2) ...
Setting up libc6:amd64 (2.24-7ubuntu3) ...
Setting up libc-bin (2.24-7ubuntu3) ...
Processing triggers for man-db (2.7.6.1-1) ...
root@zesty:~# tty
/proc/self/fd/0
root@zesty:~# echo $?
0
root@zesty:~# tmux
[exited]
root@zesty:~#

Revision history for this message
Stéphane Graber (stgraber) wrote :
Changed in glibc (Ubuntu):
importance: Undecided → High
Revision history for this message
Stéphane Graber (stgraber) wrote :

For screen, we'll need to fix their own checking logic a bit but we'll work on that patch and send it upstream (and have it also deal with just getting ENODEV while we're at it).

With the fixed glibc screen reports "Bad tty '/proc/self/fd/0'" because of it trying to readlink the path it's receiving from ttyname(), that's an unneeded step and in fact breaks on Linux when dealing with cross-namespace pts devices.

tags: added: patch
Revision history for this message
Stéphane Graber (stgraber) wrote :

After discussing this on IRC with Adam, Ubuntu will only include the upstreamed patch and we'll instead resort on patching all affected downstream pieces of software to deal with the unfortunate new glibc behavior.

Revision history for this message
Stéphane Graber (stgraber) wrote :

Adam will take care of doing an upload for Ubuntu 17.04 with 5e9a4f378c8607c2ae1aa465436af4321db0e23 included.

Changed in glibc (Ubuntu):
assignee: nobody → Adam Conrad (adconrad)
status: New → Triaged
Changed in screen (Ubuntu):
status: New → Triaged
Changed in tmux (Ubuntu):
status: New → Triaged
Changed in screen (Ubuntu):
importance: Undecided → High
Changed in tmux (Ubuntu):
importance: Undecided → High
Changed in screen (Ubuntu):
assignee: nobody → Christian Brauner (cbrauner)
Changed in tmux (Ubuntu):
assignee: nobody → Christian Brauner (cbrauner)
Revision history for this message
Stéphane Graber (stgraber) wrote :

And my team will take care of distro patching screen and tmux to work with this.

Adam Conrad (adconrad)
Changed in glibc (Ubuntu):
status: Triaged → Fix Committed
Revision history for this message
Christian Brauner (cbrauner) wrote :
Changed in screen (Ubuntu):
status: Triaged → In Progress
Changed in screen (Ubuntu):
status: In Progress → Fix Committed
Revision history for this message
Christian Brauner (cbrauner) wrote :
Changed in screen (Ubuntu):
status: Fix Committed → Triaged
Changed in screen (Ubuntu):
assignee: Christian Brauner (cbrauner) → Stéphane Graber (stgraber)
status: Triaged → Fix Committed
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package screen - 4.5.0-3ubuntu1

---------------
screen (4.5.0-3ubuntu1) zesty; urgency=medium

  * Cherry-pick bugfix for screen in containers:
    - 83_handle_pty_device_namespace.patch (LP: #1669578)

 -- Stéphane Graber <email address hidden> Fri, 24 Mar 2017 14:35:41 -0400

Changed in screen (Ubuntu):
status: Fix Committed → Fix Released
Revision history for this message
Michael Sparmann (theseven) wrote :

The fix does not seem to work, it just changes the error message to "Cannot open your terminal '' - please check." for me.
The reason seems to be that the patch relies on attach_fd to be set by code that's inside an ifndef NAMEDPIPE block, which isn't being compiled for the ubuntu package.
That ifndef has been removed from upstream code in http://git.savannah.gnu.org/cgit/screen.git/commit/?id=d965ff1d92e2bd57d24b68cfbd6990486024baa3 almost 2 years ago, but that apparently hasn't landed in the ubuntu package yet.

Revision history for this message
Axel Beckert (xtaran) wrote : Re: [Bug 1669578] Re: Get ttyname() to work properly in containers

Hi Michael,

Michael Sparmann wrote:
> That ifndef has been removed from upstream code in
> http://git.savannah.gnu.org/cgit/screen.git/commit/?id=d965ff1d92e2bd57d24b68cfbd6990486024baa3
> almost 2 years ago, but that apparently hasn't landed in the ubuntu
> package yet.

That commit is only present in the master branch which is the
developement branch for a potential Screen 5.x series.

But that branch iss far from stable. Debian and Ubuntu currently track
the screen-v4 stable branch — which still has that code.

  Regards, Axel
--
 ,''`. | Axel Beckert <email address hidden>, http://people.debian.org/~abe/
: :' : | Debian Developer, ftp.ch.debian.org Admin
`. `' | 4096R: 2517 B724 C5F6 CA99 5329 6E61 2FF9 CD59 6126 16B5
  `- | 1024D: F067 EA27 26B9 C3FC 1486 202E C09E 1D89 9593 0EDE

Revision history for this message
Christian Brauner (cbrauner) wrote :

So aside from the Ubuntu package being outdated (The codebase has indeed changed massively.) the patch we need to get screen working properly is way more intrusive then originally thought and there might even be the case that we have to do some additional lower-level plumbing in the kernel or glibc to get this working. In short, screen tries to perform various operations on the corresponding /dev/pts/<n> paths that the symbolic links under /proc/self/fd/<n> point to and it is not clear whether we can support all of that by just using the symlinks themselves.

Revision history for this message
Christian Brauner (cbrauner) wrote :

Here's a patch for screen that got things working for me. I've tested screen startup and various ways of attach (screen -r, screen -rx, and screen -rxD). Would be nice if people are able to test this. So if we could get it into proposed that be +1. The patch in its current form is directly applicable to the screen-4.5.0 Ubuntu sources. Once we consider it to be fine I will prepare a patch for master and sent it out to upstream.

Changed in screen (Ubuntu):
status: Fix Released → In Progress
assignee: Stéphane Graber (stgraber) → Christian Brauner (cbrauner)
Revision history for this message
Christian Brauner (cbrauner) wrote :

I'll likely add another patch to this soon to handle some more corner-cases.

Revision history for this message
Christian Brauner (cbrauner) wrote :
Download full text (4.0 KiB)

Here's an updated version of the patch and a comment explaining in a little more detail what is happening:

Subject: [PATCH] screen: handle pts devices in different namespaces

Various programs that deal with namespaces will use pty devices that exist in
another namespace. One obvious candiate are containers. So far ttyname() was
incorrectly handling this case because the pts device stems from the host and
thus cannot be found amongst the current namespace's /dev/pts/<n> entries.
Serge Hallyn and I recently upstreamed patches to glibc that allow
ttyname{_r}() to correctly handle this case. At a minimum, ttyname{_r}() will
set errno to ENODEV in case it finds that the /dev/pts/<n> device that the
symlink points to exists in another namespace.

(The next comment is a little longer but tries to ensure that one can still
understand what is going on after some time has passed.)
In case we detect that ttyname{_r}() returns NULL and sets errno to ENODEV we
have ample reason to assume that the pts device exists in a different
namespace. In this case, the code will set a global flag indicating this case
to true. Furthermore, all operations (e.g. chmod(), chown(), etc.) will now
need to operate on the symbolic link /proc/self/fd/0 directly. While this
sounds straightforward, it becomes difficult to handle this case correctly when
we reattach to an already existing screen session from a different pts device
than the original one. Let's look at the general reattach logic a little
closer:

Assume we are running a shell that uses a pts device from a different
namespace:

 root@zest1:~# ls -al /proc/self/fd/
 total 0
 dr-x------ 2 root root 0 Apr 2 20:22 .
 dr-xr-xr-x 9 root root 0 Apr 2 20:22 ..
 lrwx------ 1 root root 64 Apr 2 20:22 0 -> /dev/pts/6
 lrwx------ 1 root root 64 Apr 2 20:22 1 -> /dev/pts/6
 lrwx------ 1 root root 64 Apr 2 20:22 2 -> /dev/pts/6
 l-wx------ 1 root root 64 Apr 2 20:22 3 -> pipe:[3067913]
 lr-x------ 1 root root 64 Apr 2 20:22 4 -> /proc/27413/fd
 lrwx------ 1 root root 64 Apr 2 20:22 9 -> socket:[32944]

 root@zest1:~# ls -al /dev/pts/
 total 0
 drwxr-xr-x 2 root root 0 Mar 30 17:55 .
 drwxr-xr-x 8 root root 580 Mar 30 17:55 ..
 crw--w---- 1 root tty 136, 0 Mar 30 17:55 0
 crw--w---- 1 root tty 136, 1 Mar 30 17:55 1
 crw--w---- 1 root tty 136, 2 Mar 30 17:55 2
 crw--w---- 1 root tty 136, 3 Mar 30 17:55 3
 crw--w---- 1 root tty 136, 4 Mar 30 17:55 4
 crw-rw-rw- 1 root root 5, 2 Apr 2 20:22 ptmx

(As one can see /dev/pts/6 does not exist in the current namespace.)
Now, start a screen session in this shell. In this case this patch will have
screen directly operate on /proc/self/fd/0.
Let's look at the attach case. When we attach to an existing screen session
where the associated pts device lives in another namespace we need a way to
uniquely identify the pts device that is used and also need a way to get a
valid fd when we need one. This patch solves this by ensuring that a valid file
descriptor to the pts device is sent via a unix socket and SCM_RIGHTS to the
socket and display handling part of screen. However, screen also sends around
the name of the associated pts device or, in the case where the pts de...

Read more...

Changed in tmux (Ubuntu):
status: Triaged → In Progress
Revision history for this message
Christian Brauner (cbrauner) wrote :

Here's a patch for the current tmux version of Ubuntu. We should get this sorted before this Thursday.

Revision history for this message
Christian Brauner (cbrauner) wrote :

About the tmux patch. Nicholas plans to merge a patch that is nearly identical into tmux master sometime next week but this might be too late for final freeze.

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package screen - 4.5.0-5ubuntu1

---------------
screen (4.5.0-5ubuntu1) zesty; urgency=low

  * Merge from Debian unstable. Remaining changes:
    - 83_handle_pty_device_namespace.patch (LP: #1669578)

 -- Gianfranco Costamagna <email address hidden> Tue, 04 Apr 2017 12:54:26 +0200

Changed in screen (Ubuntu):
status: In Progress → Fix Released
Changed in tmux (Ubuntu):
status: In Progress → Fix Released
Changed in screen (Ubuntu):
status: Fix Released → In Progress
Revision history for this message
Christian Brauner (cbrauner) wrote :

Since the Ubuntu screen package was synced with Debian yesterday the attached patch does not apply cleanly anymore. So here's a new version of it that also is much closer to what I've pushed upstream.

Revision history for this message
Christian Brauner (cbrauner) wrote :

The tmux patch is now also upstream.

Revision history for this message
Christian Brauner (cbrauner) wrote :

The screen patch is now also upstream http://git.savannah.gnu.org/cgit/screen.git/commit/?id=565b8901cad828d921038cd6235501c42d1c9a32 . @stgraber, did you get around to pushing to to the archive as well so that we can land this in zesty?

Changed in screen (Ubuntu):
status: In Progress → Fix Committed
Revision history for this message
Christian Brauner (cbrauner) wrote :

So, Stéphane brought to my attention that we're not including the patch because it switches screen to use sockets instead of fifos and thus makes it impossible to switch to existing fifo-based screen sessions. I've written a patch that adds a compatibility layer to support both fifo-based and socket-based screen sessions. The idea is to only re-use existing fifo-sessions but never create new ones. All new screen sessions will be socket-based.

Revision history for this message
Christian Brauner (cbrauner) wrote :

I tested the screen-fifo-socket compatibility patch here https://asciinema.org/a/111692 .

Revision history for this message
Christian Brauner (cbrauner) wrote :

Updated version of fifo-socket-compat patch that removes left-behind debugging statement.

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package glibc - 2.24-9ubuntu2

---------------
glibc (2.24-9ubuntu2) zesty; urgency=medium

  * debian/patches/any/cvs-resolv-internal-qtype.diff: Revert to avoid
    failure in name resolution on upgrades from yakkety (LP: #1674532)

 -- Adam Conrad <email address hidden> Tue, 21 Mar 2017 15:27:15 -0600

Changed in glibc (Ubuntu):
status: Fix Committed → Fix Released
Revision history for this message
Christian Brauner (cbrauner) wrote :

An upstream-compatible version of 0001-add-compat-layer-to-handle-both-fifos-and-sockets.patch is now merged upstream into screen v4. See https://lists.gnu.org/archive/html/screen-devel/2017-04/msg00023.html .

Revision history for this message
Christian Brauner (cbrauner) wrote :

I sent a patch to musl upstream to align them with glibc. The patch is merged: https://git.musl-libc.org/cgit/musl/commit/?id=e1232f5b5185e8f337806841018369407e32e77d .

no longer affects: musl (Ubuntu)
Revision history for this message
Gianfranco Costamagna (costamagnagianfranco) wrote :

@cbrauner, do you have a patch that needs sponsoring?
We are discussing merging 4.5.1-3 from Debian experimental, but your patch can't apply

e.g. we have no tty.c and tty.h in our packaging.

Feel free to upload in a ppa or a patch here and I'll look/sponsor it
(anybody is free to steal the packaging from me)

Revision history for this message
Christian Brauner (cbrauner) wrote :

@LocutusOfBorg, I can give you a screen 4-5.1-3 compatible patch based on the Debian package.
One thing to note is that even my compat-layer patch will be of no help once {Debian,Ubuntu} breaks the message protocol which they eventually *will* have to do. Upstream screen uses at least message version 5 whereas {Debian,Ubuntu} use message version 2. Once you bump the message version previous screen versions will break. But updating the message protocol will be unavoidable in the long run. So you should consider warning users and start breaking backward compatibility at some point in the near future. :)

Revision history for this message
Christian Brauner (cbrauner) wrote :
Download full text (6.6 KiB)

@LocutusOfBorg, can you please tell me exactly what you need? From looking at

git clone https://anonscm.debian.org/git/collab-maint/screen.git
git checkout debian/4.5.1-3

none of the patches that I've pushed upstream are available in this repo.
For this to work you'd need:

1.
commit 78d2a73273786bd7c5f3be481b23a5bee19dc519
Author: Christian Brauner <email address hidden>
Date: Wed Mar 22 18:16:52 2017 +0100

    handle pty device from different namespace

    Various programs that deal with namespaces will use pty devices that exist in
    another namespace. One obvious candiate are containers. So far ttyname() was
    incorrectly handling this case because the pts device stems from the host and
    thus cannot be found amongst the current namespace's /dev/pts/<n> entries.
    Serge Hallyn and I recently upstreamed patches to glibc that allow ttyname{_r}()
    to correctly handle this case. At a minimum, ttyname{_r}() will set errno to
    ENODEV in case it finds that the /dev/pts/<n> device that the symlink points to
    exists in another namespace. This commit will allow screen to handle this case
    and behave correctly in a container.

    Signed-off-by: Christian Brauner <email address hidden>

2.commit 7e755ed62d311b00102b053eee88359bbf8ac2df
Author: Christian Brauner <email address hidden>
Date: Wed Apr 5 14:24:22 2017 +0200

    screen: handle pts devices in different namespaces

    Various programs that deal with namespaces will use pty devices that exist in
    another namespace. One obvious candidate are containers. So far ttyname() was
    incorrectly handling this case because the pts device stems from the host and
    thus cannot be found amongst the current namespace's /dev/pts/<n> entries.
    Serge Hallyn and I recently upstreamed patches to glibc that allow ttyname{_r}()
    to correctly handle this case. At a minimum, ttyname{_r}() will set errno to
    ENODEV in case it finds that the /dev/pts/<n> device that the symlink points to
    exists in another namespace.

    (The next comment is a little longer but tries to ensure that one can still
    understand what is going on after some time has passed.)
    In case we detect that ttyname{_r}() returns NULL and sets errno to ENODEV we
    have ample reason to assume that the pts device exists in a different
    namespace. In this case, the code will set a global flag indicating this case
    to true. Furthermore, all operations (e.g. chmod(), chown(), etc.) will now
    need to operate on the symbolic link /proc/self/fd/0 directly. While this
    sounds straightforward, it becomes difficult to handle this case correctly when
    we reattach to an already existing screen session from a different pts device
    than the original one. Let's look at the general reattach logic a little
    closer:

    Assume we are running a shell that uses a pts device from a different
    namespace:

            root@zest1:~# ls -al /proc/self/fd/
            total 0
            dr-x------ 2 root root 0 Apr 2 20:22 .
            dr-xr-xr-x 9 root root 0 Apr 2 20:22 ..
            lrwx------ 1 root root 64 Apr 2 20:22 0 -> /dev/pts/6
            lrwx------...

Read more...

Revision history for this message
Gianfranco Costamagna (costamagnagianfranco) wrote :

I had to tweak the upstream commits, because they weren't applying on top of Debian changes, and uploaded the following in my ppa.
https://launchpad.net/~costamagnagianfranco/+archive/ubuntu/locutusofborg-ppa/+sourcepub/7773744/+listing-archive-extra

can you please test it?

Revision history for this message
Christian Brauner (cbrauner) wrote :

This version of screen doesn't work in containers for me anymore.

Revision history for this message
Christian Brauner (cbrauner) wrote :

On Fri, May 5, 2017 at 12:40 PM, Christian Brauner
<email address hidden> wrote:
> This version of screen doesn't work in containers for me anymore.

@LocutusOfBorg, nevermind I tested this on Xenial which doesn't seem
to come with a version of glibc the path that Serge and I wrote. The
screen version you built with my patches included work on patched
glibc versions inside containers. From my testing with the version you
build including my patches attaching to older fifo-based screen
sessions works and new ones are all socket-based. Thanks!

Revision history for this message
Christian Brauner (cbrauner) wrote :

On Fri, May 5, 2017 at 1:34 PM, Christian Brauner
<email address hidden> wrote:
> On Fri, May 5, 2017 at 12:40 PM, Christian Brauner
> <email address hidden> wrote:
>> This version of screen doesn't work in containers for me anymore.
>
> @LocutusOfBorg, nevermind I tested this on Xenial which doesn't seem
> to come with a version of glibc the path that Serge and I wrote. The
> screen version you built with my patches included work on patched
> glibc versions inside containers. From my testing with the version you
> build including my patches attaching to older fifo-based screen
> sessions works and new ones are all socket-based. Thanks!

Fwiw, I'd like to see the glibc patch Serge and I wrote SRUed into Xenial
(and if not already done) glibc since this is likely what most users currently
use as images in their containers. @Adam, is this something we can do?

Revision history for this message
Gianfranco Costamagna (costamagnagianfranco) wrote :

I don't think we should upload without having retro-compatibility with older and still supported Ubuntu releases.

Revision history for this message
Christian Brauner (cbrauner) wrote :

On Fri, May 5, 2017 at 4:57 PM, LocutusOfBorg
<email address hidden> wrote:
> I don't think we should upload without having retro-compatibility with
> older and still supported Ubuntu releases.

What are you referring to now? The glibc patch? That doesn't break
anything otherwise
glibc upstream would have yelled at us.

If you're referring to screen . The screen version right now with my
compat-layer patch applied
should be fully backwards compatible.

>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1669578
>
> Title:
> Get ttyname() to work properly in containers
>
> Status in glibc package in Ubuntu:
> Fix Released
> Status in screen package in Ubuntu:
> Fix Committed
> Status in tmux package in Ubuntu:
> Fix Released
>
> Bug description:
> For the past year or so, the LXD team has been trying to resolve an
> issue affecting screen, tmux and a bunch of other software (including
> the "tty" command).
>
> The problem comes from the fact that when attaching to a container,
> your terminal's pts device comes from the host and therefore can't be
> found in /dev/pts/.
>
> glibc makes the assumption that it can readlink /proc/self/fd/0 and
> that the target path will exist. This simply isn't true as the symlink
> target returned by the kernel, is confusingly relative to the host's
> root and not the container's.
>
> Which means that if the target happens to exist, it's actually going
> to be an entirely different pts device from the one that you're
> actually attached to.
>
>
> You therefore need to do something along the lines of:
> - Resolve the symlink. If the target doesn't exist, return the symlink as the ttyname.
> - If the target does exist, check that its major and minor matches that of the symlink itself, if it doesn't, then return the symlink rather than the target.
>
>
> That's the ideal approach which makes existing software keep working properly without the need for any added code. After about a year of bikeshedding, the proposed glibc upstream fix has now evolved to instead returning ENODEV in the is_pty function. This allows downstream glibc users to detect this case and then use /proc/self/fd/0 rather than the return value of ttyname() but means every software using ttyname() now needs fixing.
>
> As we very much care about Ubuntu running properly inside LXD
> containers, our suggested patchset includes both the ENODEV patch as
> is still being considered by upstream (stuck on legal validation) AND
> another patch which has ttyname() return the symlink when it receives
> the ENODEV.
>
>
> We feel this is the best way to fix the problem entirely right now. Once glibc upstream merges the ENODEV side of this and all affected software get fixed upstream to deal with it, we'll then be able to drop that patch without causing any regressions.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/ubuntu/+source/glibc/+bug/1669578/+subscriptions

Revision history for this message
Gianfranco Costamagna (costamagnagianfranco) wrote :

>If you're referring to screen . The screen version right now with my
>compat-layer patch applied
>should be fully backwards compatible.

so, you mean, it didn't work before and still doesn't work with older glibc?

Revision history for this message
Christian Brauner (cbrauner) wrote :
Download full text (5.0 KiB)

On Fri, May 05, 2017 at 05:04:18PM -0000, LocutusOfBorg wrote:
> >If you're referring to screen . The screen version right now with my
> >compat-layer patch applied
> >should be fully backwards compatible.
>
> so, you mean, it didn't work before and still doesn't work with older
> glibc?

TL;DR: It should all work now without any regression
       potential. I tested your package with my patches
       applied and it works fine inside and outside of
       containers.

Additional info to summarize again what has been said in
the thread above:
1. For screen to work in containers you need a specific
   patch that Serge and I have successfully pushed into
   glibc upstream and which is already available in the
   glibc package for Ubuntu 17.04 and 17.10.
   - The glibc patch has no effect whatsoever of screen
     outside of containers.
   - The glibc patch is fully backwards compatible to
     any program out there and does not lead to
     regressions.

2. For screen to work in containers you also need the
   three patches that I wrote and pushed to upstream
   screen and which you now applied in your package.
   Thanks for that!
   - Downstream screen in {Debian,Ubuntu} communicates
     using FIFOs whereas newer upstream versions of screen
     use unix domain sockets. So far these two things have
     been mutually exclusive. However, I wrote a compat
     layer that allows screen to use both. This is my
     third patch which is also available upstream
     and now in your package.

3. **Orthogonal to all of this** I wanted to direct the
   attention of the screen maintainers to the fact that
   upstream screen uses a message protocol which is based
   on C struct that is either send around via the FIFO or
   the unix domain socket. This message protocol is
   versioned using a simple integer numbering scheme
   {1,2,3,4,5, ...}. Changing this message protocol
   is essentially an ABI break if you want to call it that.
   This means, clients that send a C struct using a
   different version than an already running screen server
   will not be able to attach to already existing sessions.
   This scenario can be postponed but it will happen once
   the screen 4.*.* series {Debian,Ubuntu} ends. There is
   no way to fix this that I know of. I just wanted to
   point this out and make the maintainers aware that
   on the next possible version bump for screen it would
   cause a break in backwards compatibility.

>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1669578
>
> Title:
> Get ttyname() to work properly in containers
>
> Status in glibc package in Ubuntu:
> Fix Released
> Status in screen package in Ubuntu:
> Fix Committed
> Status in tmux package in Ubuntu:
> Fix Released
>
> Bug description:
> For the past year or so, the LXD team has been trying to resolve an
> issue affecting screen, tmux and a bunch of other software (including
> the "tty" command).
>
> The problem comes from the fact that when attaching to a container,
> your terminal's pts device comes from the host and therefore can't be
> found in /dev/pts/.
>
> glibc mak...

Read more...

Revision history for this message
Gianfranco Costamagna (costamagnagianfranco) wrote :

@Christian, I sponsored it, but it fails the testsuite
http://autopkgtest.ubuntu.com/packages/screen

Revision history for this message
Christian Brauner (cbrauner) wrote :

@LocutusOfBorg, thanks for sponsoring I'm looking into it.

Revision history for this message
Christian Brauner (cbrauner) wrote :

Hm, the test all pass locally for me

So I downloaded the package https://launchpad.net/ubuntu/+archive/primary/+files/screen_4.5.1-3ubuntu1_amd64.deb from the proposed https://launchpad.net/ubuntu/+source/screen repo and ran the tests locally and get:

chb@conventiont|~/mnt/C/screen-debian
> screen --version
Screen version 4.05.01 (GNU) 25-Feb-17
chb@conventiont|~/mnt/C/screen-debian
> cd screen-4.5.1/debian/tests/t/
chb@conventiont|~/mnt/C/screen-debian/screen-4.5.1/debian/tests/t
> ls -a
. boilerplate.sh create-list-quit.t dash-U.t
.. command.t dash-Dm.t version-help.t
chb@conventiont|~/mnt/C/screen-debian/screen-4.5.1/debian/tests/t
> ./command.t
1..4
ok 1 - Create session
ok 2 - Detached session found
ok 3 - Session has expected session name
ok 4 - Session is gone
chb@conventiont|~/mnt/C/screen-debian/screen-4.5.1/debian/tests/t
> ./create-list-quit.t
1..6
ok 1 - Create session
ok 2 - Detached session found
ok 3 - Session has expected session name
ok 4 - Session has a window with id 0
ok 5 - Quit command sent to session
ok 6 - Session is gone
chb@conventiont|~/mnt/C/screen-debian/screen-4.5.1/debian/tests/t
> ./dash-Dm.t
1..5
ok 1 - Create session
ok 2 - Detached session found
ok 3 - Session has expected session name
ok 4 - Quit command sent to session
ok 5 - Session is gone
chb@conventiont|~/mnt/C/screen-debian/screen-4.5.1/debian/tests/t
> ./dash-U.t
1..6
ok 1 - Create session
ok 2 - Detached session found
ok 3 - Session has expected session name
ok 4 - Session has a window with id 0
ok 5 - Quit command sent to session
ok 6 - Session is gone
chb@conventiont|~/mnt/C/screen-debian/screen-4.5.1/debian/tests/t
> ./version-help.t
1..2
ok 1 - Outputs version
ok 2 - Outputs help

Revision history for this message
Gianfranco Costamagna (costamagnagianfranco) wrote :

I did get the same result (pbuilder artful clean environment)
I also did try to reproduce autopkgtestsuite in an lxc container and the test has failed

sudo autopkgtest-build-lxc ubuntu artful
autopkgtest --shell-fail --apt-upgrade screen_4.5.1-3ubuntu1.dsc -- lxc --sudo autopkgtest-artful

the problem is that when the test fails, you get a shell inside the container, and prove -v debian/tests/t works there :(
culprit might be that screen -X and screen -Q stopped working

Revision history for this message
Christian Brauner (cbrauner) wrote :

@LocutusOfBorg, thanks for taking a close look at this. Found the bug. I'm appending a patch that fixes it and makes the autopkgtestsuite pass for me!

Revision history for this message
Christian Brauner (cbrauner) wrote :

@LocutusOfBorg, the patch I appended is applicable directly to the deb source used in the package.

Revision history for this message
Christian Brauner (cbrauner) wrote :

Patch send to upstream as well.

Revision history for this message
Gianfranco Costamagna (costamagnagianfranco) wrote :

lets see if now this bug gets closed :)

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package screen - 4.5.1-3ubuntu2

---------------
screen (4.5.1-3ubuntu2) artful; urgency=medium

  [ Christian Brauner ]
  * debian/patches/0001-screen-don-t-stat.patch:
    - fix testsuite error when tty is ""

 -- Gianfranco Costamagna <email address hidden> Wed, 10 May 2017 16:59:10 +0200

Changed in screen (Ubuntu):
status: Fix Committed → Fix Released
Revision history for this message
Gianfranco Costamagna (costamagnagianfranco) wrote :

yeah :)

Revision history for this message
Axel Beckert (xtaran) wrote :

So the screen testsuite I wrote 2.5 years ago actually found a bug? Cool! :-)

Revision history for this message
Christian Brauner (cbrauner) wrote :

Wohoo, great job gentleman. :)

Revision history for this message
Jose Manuel Santamaria Lema (panfaust) wrote :

Hello,

I got this problem also in multitail but I think I know how to patch it. I will attach the debdiff as soon as I can test it properly.

Revision history for this message
Jose Manuel Santamaria Lema (panfaust) wrote :

Here's a possible fix. I tested it on bionic and it seems it works. For my xenial machines I had to workaround it disabling completely the check because the glibc there doesn't behave the same way.

So I'm attaching a debdiff with the proposed upload for bionic; it would be nice if someone could sponsor it, thanks in advance.

Revision history for this message
Launchpad Janitor (janitor) wrote :

Status changed to 'Confirmed' because the bug affects multiple users.

Changed in multitail (Ubuntu):
status: New → Confirmed
Revision history for this message
Gianfranco Costamagna (costamagnagianfranco) wrote :

@Jose Manuel Santamaria Lema (panfaust)

can you please send the patch upstream so they integrate it in a new release?
We don't get emails for bugs that are closed, and I don't understand the need of an Ubuntu only patch.
I can sponsor once you report and get it acked by upstream.

G.

To post a comment you must log in.