Bug#961655: debhelper: compat 13 HOME and XDG_RUNTIME_DIR are long enough to break AF_UNIX sockets

2020-06-28 Thread Niels Thykier
Simon McVittie:
> On Sun, 28 Jun 2020 at 11:08:36 +0200, Niels Thykier wrote:
>> [...]
> 
> The workaround in mutter[1] isn't *too* awful. The most problematic
> thing about it is that the logic for it has to be "inside" dh_auto_test;
> setting the XDG_RUNTIME_DIR in d/rules is no longer effective, because
> debhelper overrides it. This is easy enough with Meson but would be a
> pain to do in Autotools.
> 
> [...]
> 
> I wonder whether it would be viable to create an XDG_RUNTIME_DIR in /tmp
> on entry to dh_auto_test, and delete it before dh_auto_test exits? It
> seems to me that dh_auto_test is the debhelper step that is most likely
> to want to put sockets in XDG_RUNTIME_DIR. By creating a temporary
> XDG_RUNTIME_DIR before testing and deleting it afterwards, you'd
> effectively be pretending that the beginning of testing was the
> transition from 0 to 1 logins as this uid, and pretending that the end
> of testing was the transition from 1 to 0 logins, which seems like a
> reasonable mental model for what is going on.
> 
> smcv
> 
> [1] 
> https://salsa.debian.org/gnome-team/mutter/-/commit/2650f22af313674cacec7cd3dab905c032c8dc18
> 

Hi Simon,

Could you please try and review this branch?

https://salsa.debian.org/debian/debhelper/-/tree/bug-961655-XDG_RUNTIME_DIR-change

~Niels



Bug#961655: debhelper: compat 13 HOME and XDG_RUNTIME_DIR are long enough to break AF_UNIX sockets

2020-06-28 Thread Simon McVittie
On Sun, 28 Jun 2020 at 11:08:36 +0200, Niels Thykier wrote:
> Simon McVittie:
> > For now I'm going to put a workaround in mutter to create its own
> > temporary XDG_RUNTIME_DIR, but it seems a shame not to be able to take
> > advantage of debhelper solving this problem centrally.
> 
> Finally, there is the third option of rolling back this feature as
> premature and dysfunctional until a better solution can be found.  I
> doubt neither of us wants it, but I think the previous status-quo was
> better than broken builds that are hard to work around.

The workaround in mutter[1] isn't *too* awful. The most problematic
thing about it is that the logic for it has to be "inside" dh_auto_test;
setting the XDG_RUNTIME_DIR in d/rules is no longer effective, because
debhelper overrides it. This is easy enough with Meson but would be a
pain to do in Autotools.

I think this is more likely to be a problem for XDG_RUNTIME_DIR than
for HOME, because:

- the only common implementation of XDG_RUNTIME_DIR is systemd's,
  which uses a short path, meaning users of that interface do not expect
  to see longer paths;
- the only other implementation I'm aware of is Ubuntu's pam_xdg from
  before they used systemd, which also uses a short path (a different one);
- upstream developers know that home directories are
  arbitrary/uncontrolled, so are more cautious about making assumptions
  about them than about XDG_RUNTIME_DIR;
- and XDG_RUNTIME_DIR is mostly for sockets, which have the hard 108 byte
  limit, whereas HOME is mostly for regular files and directories,
  which don't

So I think even if you decide to revert the XDG_RUNTIME_DIR part of
this feature, that doesn't necessarily imply that you should revert the
HOME part too. (Also, setting your own XDG_RUNTIME_DIR is a lot simpler
than remembering that when you set your own HOME, you also have to unset
XDG_{CACHE,CONFIG,DATA}_{HOME,DIRS}, which has caught me out in the past.)

I wonder whether it would be viable to create an XDG_RUNTIME_DIR in /tmp
on entry to dh_auto_test, and delete it before dh_auto_test exits? It
seems to me that dh_auto_test is the debhelper step that is most likely
to want to put sockets in XDG_RUNTIME_DIR. By creating a temporary
XDG_RUNTIME_DIR before testing and deleting it afterwards, you'd
effectively be pretending that the beginning of testing was the
transition from 0 to 1 logins as this uid, and pretending that the end
of testing was the transition from 1 to 0 logins, which seems like a
reasonable mental model for what is going on.

smcv

[1] 
https://salsa.debian.org/gnome-team/mutter/-/commit/2650f22af313674cacec7cd3dab905c032c8dc18



Bug#961655: debhelper: compat 13 HOME and XDG_RUNTIME_DIR are long enough to break AF_UNIX sockets

2020-06-28 Thread Mattia Rizzolo
On Sun, Jun 28, 2020 at 11:08:36AM +0200, Niels Thykier wrote:
> The relevant question here being whether /tmp is cleaned automatically
> by sbuild, pbuilder (etc.) on errors.  If it is, then this approach
> might be a non-issue.

I think nothing should start relying on chroot-controllers to clean up.
Currently the Debian build process is not defined to be contained in
such setup, and as such builds are supposed to clean after themselves.


FTR, in pbuilder /tmp by default is a random directory (i.e., there is
no tmpfs or anything else mounted on it) and as such is discarded on
exit.

-- 
regards,
Mattia Rizzolo

GPG Key: 66AE 2B4A FCCF 3F52 DA18  4D18 4B04 3FCD B944 4540  .''`.
More about me:  https://mapreri.org : :'  :
Launchpad user: https://launchpad.net/~mapreri  `. `'`
Debian QA page: https://qa.debian.org/developer.php?login=mattia  `-


signature.asc
Description: PGP signature


Bug#961655: debhelper: compat 13 HOME and XDG_RUNTIME_DIR are long enough to break AF_UNIX sockets

2020-06-28 Thread Niels Thykier
Simon McVittie:
> Package: debhelper
> Version: 13.1
> Severity: important
> 
> After the release of debhelper 13.1, mutter fails to build from source
> with test failures. I can reproduce the build failure with a previous
> version of mutter that used to build successfully.
> 
> [...]
> 
> In systemd's implementation (which is more or less the reference), the real
> $XDG_RUNTIME_DIR is "/run/user/$uid", leaving 90+ bytes available for the
> suffix of an AF_UNIX socket.
> 
> Some ideas:
> 
> * Maybe use shorter path segments: debian/.dh/gen/_s/xrd instead of
>   debian/.debhelper/generated/_source/xdg-runtime-dir?
> 

The shortest, I think I can realistically get this path is about 27
characters for the debhelper part[1].  Then there is the socket name +
the build directory, which is beyond debhelper's control.

Now, the build path on buildds are based on package name and version, so
there are trivially packages that will naturally exceed the limits no
matter what I do.  E.g. firefox-esr version 68.9.0esr have the build
path, which gives something like:

  * /build/firefox-esr-kioUND/firefox-esr-68.9.0esr/ (49 chars)
  * debian/.debhelper/.t/home/ (+27 chars)
  *  (32 chars left)

With your case taking 27 chars, we are quite close to the limit as it
is.  And a +dfsg suffix (or git snapshot version) can now be the
difference between a working built and a FTBFS.  This makes me think
that we are looking at a dead end for this approach and delaying the
inevitable.

[1] I have promised to people that debian/.debhelper would be the last
time they had to update their .gitignore and I intend to stick to that
promise.

> * Maybe use a random temporary directory /tmp/dh-xrd-XX for the
>   XDG_RUNTIME_DIR instead of having it in the build directory, perhaps
>   with a symlink in debian/.debhelper to make it easier to find?
> 

That is doable and I got a poc code for that.  The major issue I see is
that there is no "clean on error" hook for debhelper.  I fear this could
be a non-trivial problem for buildds that see a non-trivial number of
failures and are long running.

So either debhelper need to unconditionally clean up the directories on
failure in each helper creating them or we need an external "on-error"
clean up hook that can assist debhelper.  Neither are currently part of
the design of the tools involved.

The relevant question here being whether /tmp is cleaned automatically
by sbuild, pbuilder (etc.) on errors.  If it is, then this approach
might be a non-issue.

> * Or maybe create a random temporary directory /tmp/dh-XX with
>   symlinks /tmp/dh-XX/xrd -> .../debian/.debhelper/... and
>   /tmp/dh-XX/home -> .../debian/.debhelper/..., and then set
>   HOME=/tmp/dh-XX/home and XDG_RUNTIME_DIR=/tmp/dh-XX/xrd
>   instead of using the physical paths? (I don't know whether that would
>   solve the problem for AF_UNIX sockets though - they might try to put
>   the canonicalized physical path in a struct sockaddr_un, rather than
>   the symlink.)
> 

AFAICT, this has the same issue as the above, except the cruft leak are
symlinks rather than directories with content.

> For now I'm going to put a workaround in mutter to create its own
> temporary XDG_RUNTIME_DIR, but it seems a shame not to be able to take
> advantage of debhelper solving this problem centrally.
> 
> smcv
> 

Finally, there is the third option of rolling back this feature as
premature and dysfunctional until a better solution can be found.  I
doubt neither of us wants it, but I think the previous status-quo was
better than broken builds that are hard to work around.

Thanks,
~Niels



Bug#961655: debhelper: compat 13 HOME and XDG_RUNTIME_DIR are long enough to break AF_UNIX sockets

2020-05-27 Thread Simon McVittie
Package: debhelper
Version: 13.1
Severity: important

After the release of debhelper 13.1, mutter fails to build from source
with test failures. I can reproduce the build failure with a previous
version of mutter that used to build successfully.

>From the build log, the failing tests are failing with errors like this:

> (actor-anchors:15901): mutter-WARNING **: 09:33:40.658: WL: error: socket 
> path 
> "/<>/debian/.debhelper/generated/_source/xdg-runtime-dir/mutter-test-display-WV9YK0"
>  plus null terminator exceeds 108 bytes
> (actor-anchors:15901): mutter-ERROR **: 09:33:40.658: Failed to create_socket

I think mutter-test-display-WV9YK0 is a Wayland socket, which are
defined to be in the $XDG_RUNTIME_DIR. AF_UNIX sockets have to fit in
a fixed-length data structure (struct sockaddr_un, which has 108 bytes
available for the path on Linux) so they can't exist in directories with
really long paths. I've had this problem in the past with Telepathy and
D-Bus unit tests that tried to use a socket in the build directory, and
resolved it there by using a socket in (a temporary directory in) /tmp.

Having a place to put AF_UNIX sockets and other rendezvous points is the
purpose of the $XDG_RUNTIME_DIR, so its absolute path ought to be short
enough to have AF_UNIX sockets with a reasonable length available for
the basename. This is also potentially an issue for $HOME, but sockets
are a smaller fraction of the purpose of $HOME, so it's perhaps less
critical there.

In systemd's implementation (which is more or less the reference), the real
$XDG_RUNTIME_DIR is "/run/user/$uid", leaving 90+ bytes available for the
suffix of an AF_UNIX socket.

Some ideas:

* Maybe use shorter path segments: debian/.dh/gen/_s/xrd instead of
  debian/.debhelper/generated/_source/xdg-runtime-dir?

* Maybe use a random temporary directory /tmp/dh-xrd-XX for the
  XDG_RUNTIME_DIR instead of having it in the build directory, perhaps
  with a symlink in debian/.debhelper to make it easier to find?

* Or maybe create a random temporary directory /tmp/dh-XX with
  symlinks /tmp/dh-XX/xrd -> .../debian/.debhelper/... and
  /tmp/dh-XX/home -> .../debian/.debhelper/..., and then set
  HOME=/tmp/dh-XX/home and XDG_RUNTIME_DIR=/tmp/dh-XX/xrd
  instead of using the physical paths? (I don't know whether that would
  solve the problem for AF_UNIX sockets though - they might try to put
  the canonicalized physical path in a struct sockaddr_un, rather than
  the symlink.)

For now I'm going to put a workaround in mutter to create its own
temporary XDG_RUNTIME_DIR, but it seems a shame not to be able to take
advantage of debhelper solving this problem centrally.

smcv