Bug#1063329: libselinux1t64: breaks system in upgrade from unstable

[Helmut Grohne]

Hi Guillem,

On Wed, Feb 07, 2024 at 04:32:45AM +0100, Guillem Jover wrote:
> Yes, I'm not sure I understand either. This is what symbol versioning
> makes possible, even providing different variants for the same symbol,
> see for example glibc or libbsd.

I think symbol versioning is subtly different and glibc does not use
symbol versioning for e.g. gettimeofday selection. With symbol
versioning, you select a default version at release time and stick to
it. In other words, building against the updated libselinux does not
allow you to use the older 32bit variant of the symbol even if you opt
out of lfs and time64 and you always get the 64bit symbol. What glibc
does is a little more fancy than my simplistic #define in that it uses
asm("name") instead. Still this approach allows for selecting which
symbol is being used via macros (e.g. _FILE_OFFSET_BITS). Please correct
me if I am misrepresenting this as my experience with symbol versioning
is fairly limited.

> In any case, if going the bi-ABI path, I think upstream should get
> involved, and the shape of this decided with them. In addition
> the library should also be built with LFS by the upstream build
> system, which it does not currently, to control its ABI.

I agree that involving upstream is a good idea and my understanding is
that someone from Canonical is doing that already, which is why the
schedule was delayed.

My real question here though is what's the downsides of providing two
variants of this symbol (whether with symbol versioning or name
redirection). From my pov, this effectively is your option 3 and what I
sketched is the most stupid implementation of it. My sketch did assume
that libselinux would be built with LFS support everywhere including
i386. Enabling that on the upstream side definitely is even better,
because it gets us to not have a Debian-specific ABI.

> I think there are only three ways to go about this, excluding the t64
> attempt:

Thanks for confirming that I've reported a real problem.

> If you'd like assistance with trying to get a proposal for 3 to
> present upstream I could look into that. But I think they should be
> involved early on to see what they'd like to see and what they might
> outright reject.

From my naive point of view, this option 3 is the clear winner. Though
it all depends on what upstream says. If upstream cooperates on any
option, that's better still as we avoid ABI deviation.

Going from here, I also looked a bit into whether we could additionally
use an upstream-cooperating approach for other packages central to
Debian to avoid t64 bumps.

pam seems difficult:
| extern time_t pam_misc_conv_warn_time; /* time that we should warn user */
| extern time_t pam_misc_conv_die_time; /* cut-off time for input */

We cannot symbol-version these in a reasonable way. All we could do is
ask upstream for a real soname bump. We have a slight advantage here: On
little endian (such as armhf), we can extend this to 64bit and 32bit
accesses will continue to work for small values. However, doing this to
m68k would break horribly. I also couldn't find any in-Debian users of
these symbols (super merely vendors pam source), so just bumping it and
accepting breakage (Guillems option 1) might be worth a go?

For libaudit1, I fail to understand why we bump it at all. Both reports
look fine to me:
https://adrien.dcln.fr/misc/armhf-time_t/2024-02-01T09:53:00/compat_reports/libaudit-dev/base_to_lfs/compat_report.html
https://adrien.dcln.fr/misc/armhf-time_t/2024-02-01T09:53:00/compat_reports/libaudit-dev/lfs_to_time_t/compat_report.html
This does not extend to libauparse0 where the report gives a reason, but
libaudit1 is the one that interacts with /usr-move and libauparse0 not,
so can we skip the dance for libaudit1?

For libtirpc, it is only about rpcb_gettime, which returns time via a
time_t* and can indicate success/failure via return. It seems fairly
simple to implement ABI duality here and libtirpc already does symbol
versioning. Maybe we can also approach upstream about this?

For libfuse2, I think the ABI analysis is broken. The base-to-lfs report
supposedly is ok
https://adrien.dcln.fr/misc/armhf-time_t/2024-02-01T09:53:00/compat_reports/libfuse-dev/base_to_lfs/compat_report.html
and then going lfs-to-time changes ino_t
https://adrien.dcln.fr/misc/armhf-time_t/2024-02-01T09:53:00/compat_reports/libfuse-dev/lfs_to_time_t/compat_report.html
while I would have expected ino_t to change with lfs already. Are we
sure about this? In any case, this is more of an academic question as
adding ABI-duality would be more involved here. Moreover, I don't see
any ACC report for libfuse3-dev. Did that fail to analyze?

libiw30 only has one affected symbol:
iw_print_timeval ( char* buffer, int buflen, struct timeval const* time, struct timezone const* tz )
Providing ABI duality for this seems doable. Moreover, libiw30 already
has soname 30, so maybe upstream is open to bumping it again? The
resulting library transition is fairly small.

ntfs-3g might be worth a second look. It does use time_t and timeval
quite a bit, but it seems to do so in inline functions from ntfstime.h
and internally use a "typedef sle64 ntfs_time;", so the library might
actually be unaffected and automatically provide ABI-duality via inline
functions! The lfs side looks less bright as e.g. FILEREADER embeds
off_t.

I also looked into a few more libraries affected by both /usr-move and
time64 and figured that none of the others seems worth a deeper look. In
general though approaching upstreams for doing a soname bump to
accommodate lfs+time64 seems like a reasonable thing to ask and maybe
like 10% agree?

Candidates approaching for soname bumps:
* libeinfo1
* libgsmme1
* libiv-unidraw2
* libparted-fs-resize0
* libparted2
* librc1

Also when looking into effort, please keep in mind that for every case
mentioned in this email, we're looking into adding a protective
diversion due to the package rename without so rename and then in forky
we'll have to clean up all those diversions, and in forky+1 we'll have
to delete the cleanup code, so while investing more now may seem more
expensive, it saves later.

Helmut

[Andreas Metzler]

On 2024-02-06 Helmut Grohne <hel...@subdivi.de> wrote:
> Package: libselinux1t64

[...]> This looks fairly innocuous. We create a minimal sid chroot and install
> libselinux1t64 using apt. What could possibly go wrong? Well, apt thinks
> that it would be a good idea to avoid coinstalling breaking packages and
> first removes libselinux1 before proceeding to install libselinux1t64.
> Unfortunately, libselinux1 is transitively essential and dpkg links it,
[...]
> both dpkg and tar are now broken. This is pretty bad. To make matters
> worse, the situation arises from the combination of Breaks + Provides
[...]

Hello,

color me stupid but isn't this fishy?

Package: libselinux1t64
Replaces: libselinux1
Provides: libselinux1 (= 3.5-2.1~exp1)
Breaks: libselinux1 (<< 3.5-2.1~exp1)

Afaiui libselinux1t64 must not fullfill dpkg 1.22.4's dependency on
"libselinux1 (>= 3.1~)". dpkg needs to be rebuilt and the rebuilt
version gets a dep on "libselinux1t64 (>= 3.5)".

Will ${t64:Provides} stop expanding to "libselinux1 = ${binary:Version
for real t64-builds? (The ones in experimental are not.) If that is case
this bug and this way of testing does not make sense.

Otherwise the plan looks flawed.

cu Andreas
--
`What a good friend you are to him, Dr. Maturin. His other friends are
so grateful to you.'
`I sew his ears on from time to time, sure'

[Michael Tokarev]

07.02.2024 11:06, Helmut Grohne :
..

> pam seems difficult:
> | extern time_t pam_misc_conv_warn_time; /* time that we should warn user */
> | extern time_t pam_misc_conv_die_time; /* cut-off time for input */

Attached is a sketch to make pam compatible.

I had a more complete and *tested* fix 2 days ago but I forgot
it was in /tmp and I rebooted the machine, so had to do it again
yesterday.

The idea is to have both die_time and die_time64, and keep them
in sync (using minimum between two values which is !=0).

This is a sketch using a #define, though better is to use symbol
versioning here and have time32 compat stuff for old programs
and 64bit time stuff for new, using redirection at the link time,
instead of the #defines which makes whole thing rather difficult
to read, - that's extra several lines of code, also to the .map
file.

What the whole thing needs is the criteria to use to enable the
trick. Right now I used #ifdef NEED_TIME64_COMPAT which should
be defined somehow, - since I don't know the precise list of
architectures where this has to be done. This is an externally-
controlled thing, there's no way to determine this directly from
the .c code (short of using architecture list in the .h file),
so it must be some symbol substituted at the package build time,
like Provides: t64:Compat (or whatever it is) is substituted in
d/control.

This is a less-intrusve-to-original-code version of the sketch,
ie, I tried to keep all changes outside of the original code as
possible, keeping all the original logic as it is.

/mjt

[Steve Langasek]

Replying separately to issues unrelated to libselinux, dropping the bug from
cc: and picking a possibly better subject.

First, I want to double check here: the libselinux bug was identified as a
showstopper because removal of the file from disk even briefly breaks dpkg
itself, making recovery impossible. The other libraries mentioned in this
thread, though transitively essential or at least Priority: required, are
not dependencies of apt and so do not have this impact.

So is the consensus that these other libraries also all need solutions that
avoid package renames?

Of course, we should look for the best possible technical solution here, but
not at the cost of indefinitely dragging out the transition for issues that
aren't showstoppers.

On Wed, Feb 07, 2024 at 09:06:58AM +0100, Helmut Grohne wrote:
> pam seems difficult:
> | extern time_t pam_misc_conv_warn_time; /* time that we should warn user */
> | extern time_t pam_misc_conv_die_time; /* cut-off time for input */

> We cannot symbol-version these in a reasonable way. All we could do is
> ask upstream for a real soname bump. We have a slight advantage here: On
> little endian (such as armhf), we can extend this to 64bit and 32bit
> accesses will continue to work for small values. However, doing this to
> m68k would break horribly. I also couldn't find any in-Debian users of
> these symbols (super merely vendors pam source), so just bumping it and
> accepting breakage (Guillems option 1) might be worth a go?

Since these are basically part of a 30-year-old spec that predates Linux PAM
and there doesn't seem to really be any real-world use of these, IF we
decide that a package rename for libpam0g is unacceptable, then I think
option 1 is ok. The impact would be corner case runtime misbehavior of
applications depending on being able so set a non-default timeout (the
default value here is 0), not application crashes.

We could potentially "clean" this up after the fact as well by detecting
cases where the low 32-bits are 0 on big-endian archs and word-swap, which
would keep those hypothetical applications working as-is without recompile
until 2038.

> For libaudit1, I fail to understand why we bump it at all. Both reports
> look fine to me:
> https://adrien.dcln.fr/misc/armhf-time_t/2024-02-01T09:53:00/compat_reports/libaudit-dev/base_to_lfs/compat_report.html
> https://adrien.dcln.fr/misc/armhf-time_t/2024-02-01T09:53:00/compat_reports/libaudit-dev/lfs_to_time_t/compat_report.html
> This does not extend to libauparse0 where the report gives a reason, but
> libaudit1 is the one that interacts with /usr-move and libauparse0 not,
> so can we skip the dance for libaudit1?

Thanks. The issue is that in cases of a source package with multiple header
packages and multiple runtime packages, there is no way to consistently and
reliably map between the two, so we conservatively assume that any -dev
package in the source package whose ABI is affected impacts all runtime
libraries.

(In this specific case there's enough information that we *could*
programmatically identify that libaudit-dev depends libaudit1 but not
libauparse0, libauparse-dev depends libauparse0 and not libaudit1, and the
dev packages do not depend on one another; however, this was not
implemented.)

Since eyeballing this clearly shows that only libauparse0 has an impacted
ABI, I will do a follow-up upload to experimental correcting this.

> For libtirpc, it is only about rpcb_gettime, which returns time via a
> time_t* and can indicate success/failure via return. It seems fairly
> simple to implement ABI duality here and libtirpc already does symbol
> versioning. Maybe we can also approach upstream about this?

This is the only mention of libtirpc in this thread; it is not Essential:
yes or Priority: required. The bug you filed about inadequate mitigation in
experimental is closed. So I'm not sure why this is on the list here,
unless your concern is that we should aim to avoid ALL such maintainer
script mitigations for file moves from / to /usr and have a blanket ban on
the renames?

> For libfuse2, I think the ABI analysis is broken. The base-to-lfs report
> supposedly is ok
> https://adrien.dcln.fr/misc/armhf-time_t/2024-02-01T09:53:00/compat_reports/libfuse-dev/base_to_lfs/compat_report.html
> and then going lfs-to-time changes ino_t
> https://adrien.dcln.fr/misc/armhf-time_t/2024-02-01T09:53:00/compat_reports/libfuse-dev/lfs_to_time_t/compat_report.html
> while I would have expected ino_t to change with lfs already. Are we
> sure about this? In any case, this is more of an academic question as
> adding ABI-duality would be more involved here.

Are we *sure* about it, no. But I'm not sure it's worth the effort to get a
definitive answer here. libfuse2 should be deprecated in favor of libfuse3
and if there's pushback I'd rather just allow it to be broken for 64-bit
time_t and use this as a forcing factor to complete the transition, rather
than putting effort into fixing libfuse2. (the number of
reverse-dependencies is unfortunately still quite large.)

> Moreover, I don't see
> any ACC report for libfuse3-dev. Did that fail to analyze?

Yes:

https://adrien.dcln.fr/misc/armhf-time_t/2024-02-09T15%3A12%3A00/logs/libfuse3-dev/time_t/log.txt

looks like a bug in the quirking script. The same bug appears to be present
for libhiredis-dev.

> Also when looking into effort, please keep in mind that for every case
> mentioned in this email, we're looking into adding a protective
> diversion due to the package rename without so rename and then in forky
> we'll have to clean up all those diversions, and in forky+1 we'll have
> to delete the cleanup code, so while investing more now may seem more
> expensive, it saves later.

I think the cost of chasing upstreams about soname bumps and dual-abi'ing
libraries swamps any savings for the maintainers having to do lazy cleanup
of some lingering diversions.

Thanks,
--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer https://www.debian.org/
slan...@ubuntu.com vor...@debian.org

[Adrien Nader]

Hi,

On Thu, Feb 15, 2024, Steve Langasek wrote:
> > For libfuse2, I think the ABI analysis is broken. The base-to-lfs report
> > supposedly is ok
> > https://adrien.dcln.fr/misc/armhf-time_t/2024-02-01T09:53:00/compat_reports/libfuse-dev/base_to_lfs/compat_report.html
> > and then going lfs-to-time changes ino_t
> > https://adrien.dcln.fr/misc/armhf-time_t/2024-02-01T09:53:00/compat_reports/libfuse-dev/lfs_to_time_t/compat_report.html
> > while I would have expected ino_t to change with lfs already. Are we
> > sure about this? In any case, this is more of an academic question as
> > adding ABI-duality would be more involved here.
>
> Are we *sure* about it, no. But I'm not sure it's worth the effort to get a
> definitive answer here. libfuse2 should be deprecated in favor of libfuse3
> and if there's pushback I'd rather just allow it to be broken for 64-bit
> time_t and use this as a forcing factor to complete the transition, rather
> than putting effort into fixing libfuse2. (the number of
> reverse-dependencies is unfortunately still quite large.)

fuse.pc uses -D_FILE_OFFSET_BITS=64 and I see no reason it would apply
only when doing two of the ABI dumps and not the third one.
Abi-compliance-checker reports type changes but not size changes so I'm
inclined to think it's just hitting different paths in the glibc headers
and the type name is different (which makes sense since there is no
non-lfs in time64).

> > Moreover, I don't see
> > any ACC report for libfuse3-dev. Did that fail to analyze?
>
> Yes:
>
> https://adrien.dcln.fr/misc/armhf-time_t/2024-02-09T15%3A12%3A00/logs/libfuse3-dev/time_t/log.txt

The analysis is fixed now and no ABI change has been detected (for LFS
or for time_t).

> looks like a bug in the quirking script. The same bug appears to be present
> for libhiredis-dev.

Analysis is fixed too and no ABI change has been detected for it either.

I'm not publishing consolidated results yet; unless there is a need
before, I'll do it on Friday.

--
Adrien

[Steve Langasek]

Thanks for this patch. Based on the rest of the discussion I don't believe
this is something we need in Debian. Do you want to propose this to
upstream?

> diff --git a/libpam_misc/include/security/pam_misc.h b/libpam_misc/include/security/pam_misc.h
> index fca2422..922341c 100644
> --- a/libpam_misc/include/security/pam_misc.h
> +++ b/libpam_misc/include/security/pam_misc.h
> @@ -21,6 +21,15 @@ extern int misc_conv(int num_msg, const struct pam_message **msgm,
>
> #include <time.h>
>
> +#if NEED_TIME64_COMPAT
> +
> +extern time32_t pam_misc_conv_warn_time;
> +extern time32_t pam_misc_conv_die_time;
> +#define pam_misc_conv_warn_time pam_misc_conv_warn_time64
> +#define pam_misc_conv_die_time pam_misc_conv_die_time64
> +
> +#endif
> +

> extern time_t pam_misc_conv_warn_time; /* time that we should warn user */
> extern time_t pam_misc_conv_die_time; /* cut-off time for input */

> extern const char *pam_misc_conv_warn_line; /* warning notice */
> diff --git a/libpam_misc/misc_conv.c b/libpam_misc/misc_conv.c
> index 908ee89..a02d491 100644
> --- a/libpam_misc/misc_conv.c
> +++ b/libpam_misc/misc_conv.c
> @@ -30,6 +30,9 @@
> time_t pam_misc_conv_warn_time = 0; /* time when we warn */
> time_t pam_misc_conv_die_time = 0; /* time when we timeout */
>
> +static void fixup_compat_time();
> +static void reset_warn_time();
> +
> const char *pam_misc_conv_warn_line = N_("...Time is running out...\n");
> const char *pam_misc_conv_die_line = N_("...Sorry, your time is up!\n");
>
> @@ -96,6 +99,8 @@ static int get_delay(void)
> expired = 0; /* reset flag */
> (void) time(&now);
>
> + fixup_compat_time();
> +
> /* has the quit time past? */
> if (pam_misc_conv_die_time && now >= pam_misc_conv_die_time) {
> fprintf(stderr,"%s",pam_misc_conv_die_line);
> @@ -107,7 +112,7 @@ static int get_delay(void)
> /* has the warning time past? */
> if (pam_misc_conv_warn_time && now >= pam_misc_conv_warn_time) {
> fprintf(stderr, "%s", pam_misc_conv_warn_line);
> - pam_misc_conv_warn_time = 0; /* reset warn_time */
> + reset_warn_time();
>
> /* indicate remaining delay - if any */
>
> @@ -399,3 +404,36 @@ failed_conversation:
>
> return PAM_CONV_ERR;
> }
> +
> +#ifdef NEED_TIME64_COMPAT
> +
> +#undef pam_misc_conv_warn_time
> +#undef pam_misc_conv_die_time
> +
> +int pam_misc_conv_warn_time, pam_misc_conv_die_time;
> +
> + /* in compat 32/64 case, we have 2 values: time and time64.
> + We perform all operations with time64
> + But we try to keep them in sync, whiciever is smaller but !0 */
> +#define fixup(t, t32) \
> + if (t32 && (!t || t > t32)) t = t32; /* if t32 fires sooner */ \
> + if (t < 0x7fffffff) t32 = t /* only if t can fit in t32 */
> +
> +static void fixup_compat_time() {
> + fixup(pam_misc_conv_warn_time64, pam_misc_conv_warn_time);
> + fixup(pam_misc_conv_die_time64, pam_misc_conv_die_time);
> +}
> +static void reset_warn_time() {
> + pam_misc_conv_warn_time = 0;
> + pam_misc_conv_warn_time64 = 0;
> +}
> +
> +#else
> +
> +static void fixup_compat_time() {
> +}
> +static void reset_warn_time() {
> + pam_misc_conv_warn_time = 0;
> +}
> +
> +#endif