Re: [PATCH] ldconfig: set LC_COLLATE to C

[Aurelien Jarno <aurelien@aurel32.net>]

On 2017-11-27 08:36, Carlos O'Donell wrote:
> On 11/26/2017 03:32 AM, Aurelien Jarno wrote:
> > ldconfig supports "include" directives and use the glob function to
> > process them. The glob function sort entries according to the LC_COLLATE
> > category. When using a standard "include /etc/ld.so.conf.d/*.conf" entry
> > in /etc/ld.so.conf, the order therefore depends on the locale used to
> > ldconfig. Prefixing the files that have to be processed last with "z"
> > or "zz" (as it is often used) therefore doesn't work correctly as "z"
> > is not always the last letter. For example in the et_EE locale, it is
> > sorted after the "s".
> > 
> > This patch fixes that by setting LC_COLLATE to C in order to process
> > files in deterministic order, independently of the locale used to launch
> > ldconfig.
> 
> This patch, while being the correct thing to do, has the capability to
> completely break a users system if the administrator used locale-specific
> ordering to find paths that were needed for say critical identity management
> plugins for ldap or other subsystems required for login (krb5), of which 
> I know at least one with such changes (but no locale-specific ordering on
> the configuration files).
> 
> A quick brainstorm:
> 
> * Do nothing. A minimal number of users have broken systems because of collation
>   in their locale breaks certain packages adding known to sort incorrectly entries
>   from their distribution for ld.so.conf.d/.

Note that even doing nothing, we might break (or unbreak) systems by
fixing collation in locales. There have been a lot of good work in that
area recently.

> * Switch to C collation and have an unknown number of users with broken systems
>   because they used locale specific sorting for files they added to the
>   ld.so.conf.d/ directory to configure their systems.
> 
> We should never have been supporting non-C/POSIX collation in this case, because
> collations for languages changes over time. The Postgresql upstream project is
> learning this because as glibc locales change their index files are invalidated
> because they use strcoll. We really should switch to C.UTF-8, but we don't have
> this upstream yet (working on it).

Even if C.UTF-8 is not supported upstream yet, a few distributions (at
least Fedora, Debian and Ubuntu) support it. If we want to support
unicode characters, but still having a fixed order, it might be a good
idea to add a comment to the code 1) to not forget to do the change at
some point 2) to tell such distribution they might already use C.UTF-8
there.

> Therefore the safest fix is to switch to C/POSIX collation for now (or forever).
> 
> We *could* add a flag to ldconfig to return the old behaviour, and distros deploying
> 2.27 could have the option to enable that via a wrapper script, but I doubt it
> would be used. It might have value for some kind of continuous deployment where you
> want to work around such ld.so.conf.d/ scriplets that don't sort properly, but 
> you might as well just fix those instead of work around them.
> 
> Users with broken systems will only see the breakage if they install such a new
> glibc, and then run ldconfig, and that is a big change for a stable system. Therefore
> I think this will be an OK change, but it *must not* be backported to stable branches
> since it changes expected behaviour.
> 
> > ---
> >  elf/ldconfig.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/elf/ldconfig.c b/elf/ldconfig.c
> > index 89042351f8..2c01ab567b 100644
> > --- a/elf/ldconfig.c
> > +++ b/elf/ldconfig.c
> > @@ -1259,6 +1259,10 @@ main (int argc, char **argv)
> >    /* Set locale via LC_ALL.  */
> >    setlocale (LC_ALL, "");
> >  
> > +  /* But keep the C collation.  That way `include' directives using
> > +     globbing patterns are processed in a locale-independent order.  */
> > +  setlocale (LC_COLLATE, "C");
> > +
> >    /* Set the text message domain.  */
> >    textdomain (_libc_intl_domainname);
> 
> * Needs a bug. This is is a user visible feature (Joseph just commented this too).

Indeed, I'll do that now.

> * Needs a NEWS entry.
> 
> * Needs a changelog.
> 
> * Needs a packaging change doc entry: 
>   https://sourceware.org/glibc/wiki/Release/2.27#Packaging_Changes

I'll do that in the next version once we have converged on how to flip
the switch.

> OK with those changes.
> 
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>

Thanks for the review.

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net