[PATCH] ldconfig: set LC_COLLATE to C

[PATCH] ldconfig: set LC_COLLATE to C

[Aurelien Jarno]

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

Re: [PATCH] ldconfig: set LC_COLLATE to C

[Rical Jasan]

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

Did you mean before the "s"?

But wouldn't the user/admin expect the files to be sorted according to
their locale?  If a strict ordering is needed, I would expect numerical
prefixing to be used.

Rical

Re: [PATCH] ldconfig: set LC_COLLATE to C

[Aurelien Jarno]

On 2017-11-28 00:47, Rical Jasan 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".
> 
> Did you mean before the "s"?

More precisely just after the "s" or between the "s" and the "t".

> But wouldn't the user/admin expect the files to be sorted according to
> their locale?

There might be more than one admin on the system using different
locales. In addition to that ldconfig can be launched by system scripts
using a different locale.

> If a strict ordering is needed, I would expect numerical
> prefixing to be used.

In practice distributions ship file in /etc/ld.so.conf.d, so it's not
something fully in control of users. On Debian and Fedora for example
they start with letters. And depending on the locale, numerical prefixes
might be sorted before or after letters.

Aurelien

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

Re: [PATCH] ldconfig: set LC_COLLATE to C

[Andreas Schwab]

On Nov 28 2017, Rical Jasan <ricaljasan@pacific.net> wrote:

> But wouldn't the user/admin expect the files to be sorted according to
> their locale?  If a strict ordering is needed, I would expect numerical
> prefixing to be used.

I don't think the contents of the (system-wide) ld.so cache should
depend on the locale of the admin.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] ldconfig: set LC_COLLATE to C

[Joseph Myers]

This seems like it's fixing an issue that would have been user-visible in 
releases, so should have a bug filed in Bugzilla.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] ldconfig: set LC_COLLATE to C

[Carlos O'Donell]

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

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

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

* Needs a NEWS entry.

* Needs a changelog.

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

OK with those changes.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

-- 
Cheers,
Carlos.

Re: [PATCH] ldconfig: set LC_COLLATE to C

[Florian Weimer]

On 11/27/2017 05:36 PM, Carlos O'Donell wrote:
> 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/.
> 
> * 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 can also enumerate twice and warn if there are any differences.

Or provide a tool like (glibc-system-check?) which reports this and 
could be extended over time to also check for use of deprecated 
features/APIs and so on.

Thanks,
Florian

Re: [PATCH] ldconfig: set LC_COLLATE to C

[Carlos O'Donell]

On 11/27/2017 09:02 AM, Florian Weimer wrote:
> On 11/27/2017 05:36 PM, Carlos O'Donell wrote:
>> 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/.
>>
>> * 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 can also enumerate twice and warn if there are any differences.

Are you suggesting a 2 release staged change?

* First release compare both collations, warn on differences.

* Change the release default, and stop warning.

It would not make sense to keep warning after the change.
 
> Or provide a tool like (glibc-system-check?) which reports this and
> could be extended over time to also check for use of deprecated
> features/APIs and so on.

Would we use the tool to block upgrades?

Detect /etc/ld.so.conf.d/* file names that would sort differently
given the current locale and C locale, and return non-zero with
a warning?

-- 
Cheers,
Carlos.

Re: [PATCH] ldconfig: set LC_COLLATE to C

[Florian Weimer]

On 11/27/2017 06:15 PM, Carlos O'Donell wrote:
> On 11/27/2017 09:02 AM, Florian Weimer wrote:
>> On 11/27/2017 05:36 PM, Carlos O'Donell wrote:
>>> 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/.
>>>
>>> * 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 can also enumerate twice and warn if there are any differences.
> 
> Are you suggesting a 2 release staged change?
> 
> * First release compare both collations, warn on differences.
> 
> * Change the release default, and stop warning.
> 
> It would not make sense to keep warning after the change.

Why do you say that?

System administrators might see the warning and realize immediately that 
this is an important change.

>> Or provide a tool like (glibc-system-check?) which reports this and
>> could be extended over time to also check for use of deprecated
>> features/APIs and so on.
> 
> Would we use the tool to block upgrades?
> 
> Detect /etc/ld.so.conf.d/* file names that would sort differently
> given the current locale and C locale, and return non-zero with
> a warning?

No, I think it would have to be purely advisory.

Thanks,
Florian

Re: [PATCH] ldconfig: set LC_COLLATE to C

[Carlos O'Donell]

On 11/27/2017 09:16 AM, Florian Weimer wrote:
> On 11/27/2017 06:15 PM, Carlos O'Donell wrote:
>> On 11/27/2017 09:02 AM, Florian Weimer wrote:
>>> On 11/27/2017 05:36 PM, Carlos O'Donell wrote:
>>>> 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/.
>>>>
>>>> * 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 can also enumerate twice and warn if there are any differences.
>>
>> Are you suggesting a 2 release staged change?
>>
>> * First release compare both collations, warn on differences.
>>
>> * Change the release default, and stop warning.
>>
>> It would not make sense to keep warning after the change.
> 
> Why do you say that?
> 
> System administrators might see the warning and realize immediately that this is an important change.

Once the change is made to use C/POSIX locale for collation, the warning
becomes a false positive. Why issue a warning at that point? No change is
required by the system administrator.

The warning is important leading up to the change to notify the system
administrator that the system behaviour is about to change in the future
(particularly if they are on a 2.27-based release and will pickup this
change soon e.g. Fedora Rawhide).

>>> Or provide a tool like (glibc-system-check?) which reports this and
>>> could be extended over time to also check for use of deprecated
>>> features/APIs and so on.
>>
>> Would we use the tool to block upgrades?
>>
>> Detect /etc/ld.so.conf.d/* file names that would sort differently
>> given the current locale and C locale, and return non-zero with
>> a warning?
> 
> No, I think it would have to be purely advisory.

OK.

Given my comments above, do you recommend a 2 release change process?

* Deprecate with warnings.

* Change default, and remove warning.

-- 
Cheers,
Carlos.

Re: [PATCH] ldconfig: set LC_COLLATE to C

[Aurelien Jarno]

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