mailing list of musl libc
 help / color / mirror / code / Atom feed
* Re: First feedback on new C locale problems
@ 2015-09-26  4:58 Felix Janda
  2015-09-26 19:35 ` Rich Felker
  0 siblings, 1 reply; 11+ messages in thread
From: Felix Janda @ 2015-09-26  4:58 UTC (permalink / raw)
  To: musl

On 2015-09-09 05:56:48 GMT, Rich Felker wrote:
> On Tue, Sep 01, 2015 at 02:32:35AM -0400, Rich Felker wrote:
> > What I'd like to do to fix it is just always return "UTF-8" for
> > nl_langinfo(CODESET) regardless of locale (rather than returning
> > "UTF-8-CODE-UNITS" when in C locale). POSIX places no requirements on
> > nl_langinfo that would preclude this, and it seems like it would
> > restore the desired properties and fix all the regressions.
>
> Committed.
>
> Rich

GNU sed seems to care about the output from nl_langinfo:

https://bugs.gentoo.org/show_bug.cgi?id=560728

More specifically, so does lib/localecharset.c, which is used in
the replacement of re_compile_pattern.

Felix


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Re: First feedback on new C locale problems
  2015-09-26  4:58 First feedback on new C locale problems Felix Janda
@ 2015-09-26 19:35 ` Rich Felker
  2015-09-27  6:17   ` Felix Janda
  0 siblings, 1 reply; 11+ messages in thread
From: Rich Felker @ 2015-09-26 19:35 UTC (permalink / raw)
  To: musl

On Sat, Sep 26, 2015 at 06:58:36AM +0200, Felix Janda wrote:
> On 2015-09-09 05:56:48 GMT, Rich Felker wrote:
> > On Tue, Sep 01, 2015 at 02:32:35AM -0400, Rich Felker wrote:
> > > What I'd like to do to fix it is just always return "UTF-8" for
> > > nl_langinfo(CODESET) regardless of locale (rather than returning
> > > "UTF-8-CODE-UNITS" when in C locale). POSIX places no requirements on
> > > nl_langinfo that would preclude this, and it seems like it would
> > > restore the desired properties and fix all the regressions.
> >
> > Committed.
> >
> > Rich
> 
> GNU sed seems to care about the output from nl_langinfo:
> 
> https://bugs.gentoo.org/show_bug.cgi?id=560728
> 
> More specifically, so does lib/localecharset.c, which is used in
> the replacement of re_compile_pattern.

I was able to reproduce this (with slightly different output, "a© a'")
on Alpine. Clearly this is some sort of bug in the gnulib code or sed
itself, since it's producing corrupt output. I think we should explore
why that's happening and whether it's possible to fix there. But if
there remain other reasons that returning "UTF-8" in the C locale is
not practical then perhaps we could resort to returning "ASCII".

Rich


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Re: First feedback on new C locale problems
  2015-09-26 19:35 ` Rich Felker
@ 2015-09-27  6:17   ` Felix Janda
  2015-09-27 13:47     ` Rich Felker
  0 siblings, 1 reply; 11+ messages in thread
From: Felix Janda @ 2015-09-27  6:17 UTC (permalink / raw)
  To: musl

Rich Felker wrote:
> On Sat, Sep 26, 2015 at 06:58:36AM +0200, Felix Janda wrote:
> > On 2015-09-09 05:56:48 GMT, Rich Felker wrote:
> > > On Tue, Sep 01, 2015 at 02:32:35AM -0400, Rich Felker wrote:
> > > > What I'd like to do to fix it is just always return "UTF-8" for
> > > > nl_langinfo(CODESET) regardless of locale (rather than returning
> > > > "UTF-8-CODE-UNITS" when in C locale). POSIX places no requirements on
> > > > nl_langinfo that would preclude this, and it seems like it would
> > > > restore the desired properties and fix all the regressions.
> > >
> > > Committed.
> > >
> > > Rich
> > 
> > GNU sed seems to care about the output from nl_langinfo:
> > 
> > https://bugs.gentoo.org/show_bug.cgi?id=560728
> > 
> > More specifically, so does lib/localecharset.c, which is used in
> > the replacement of re_compile_pattern.
> 
> I was able to reproduce this (with slightly different output, "a© a'")
> on Alpine. Clearly this is some sort of bug in the gnulib code or sed
> itself, since it's producing corrupt output. I think we should explore
> why that's happening and whether it's possible to fix there. But if
> there remain other reasons that returning "UTF-8" in the C locale is
> not practical then perhaps we could resort to returning "ASCII".

A possible fix is

--- ./a/sed-4.2.1/lib/regcomp.c
+++ ./a/sed-4.2.1/lib/regcomp.c
@@ -824,7 +824,7 @@ re_compile_internal (regex_t *preg, cons
 
 #ifdef RE_ENABLE_I18N
   /* If possible, do searching in single byte encoding to speed things up.  */
-  if (dfa->is_utf8 && dfa->mb_cur_max != 1 && !(syntax & RE_ICASE) && preg->translate == NULL)
+  if (dfa->is_utf8 && !(syntax & RE_ICASE) && preg->translate == NULL)
     optimize_utf8 (dfa);
 #endif
 

In our case is_utf8 is 1 and mb_cur_max is also 1. The function
optimize_utf8() would change "." to match utf8 characters instead of
bytes. For some reason I have not investigated further then "©" (or any
other non-ASCII) character is not matched, but in the C locale we want
"." also to match non-valid utf8 characters anyway.

glibc seems to be the upstream for the code.

Felix


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Re: First feedback on new C locale problems
  2015-09-27  6:17   ` Felix Janda
@ 2015-09-27 13:47     ` Rich Felker
  2015-09-27 13:49       ` Felix Janda
  0 siblings, 1 reply; 11+ messages in thread
From: Rich Felker @ 2015-09-27 13:47 UTC (permalink / raw)
  To: musl

On Sun, Sep 27, 2015 at 08:17:38AM +0200, Felix Janda wrote:
> Rich Felker wrote:
> > On Sat, Sep 26, 2015 at 06:58:36AM +0200, Felix Janda wrote:
> > > On 2015-09-09 05:56:48 GMT, Rich Felker wrote:
> > > > On Tue, Sep 01, 2015 at 02:32:35AM -0400, Rich Felker wrote:
> > > > > What I'd like to do to fix it is just always return "UTF-8" for
> > > > > nl_langinfo(CODESET) regardless of locale (rather than returning
> > > > > "UTF-8-CODE-UNITS" when in C locale). POSIX places no requirements on
> > > > > nl_langinfo that would preclude this, and it seems like it would
> > > > > restore the desired properties and fix all the regressions.
> > > >
> > > > Committed.
> > > >
> > > > Rich
> > > 
> > > GNU sed seems to care about the output from nl_langinfo:
> > > 
> > > https://bugs.gentoo.org/show_bug.cgi?id=560728
> > > 
> > > More specifically, so does lib/localecharset.c, which is used in
> > > the replacement of re_compile_pattern.
> > 
> > I was able to reproduce this (with slightly different output, "a© a'")
> > on Alpine. Clearly this is some sort of bug in the gnulib code or sed
> > itself, since it's producing corrupt output. I think we should explore
> > why that's happening and whether it's possible to fix there. But if
> > there remain other reasons that returning "UTF-8" in the C locale is
> > not practical then perhaps we could resort to returning "ASCII".
> 
> A possible fix is
> 
> --- ./a/sed-4.2.1/lib/regcomp.c
> +++ ./a/sed-4.2.1/lib/regcomp.c
> @@ -824,7 +824,7 @@ re_compile_internal (regex_t *preg, cons
>  
>  #ifdef RE_ENABLE_I18N
>    /* If possible, do searching in single byte encoding to speed things up.  */
> -  if (dfa->is_utf8 && dfa->mb_cur_max != 1 && !(syntax & RE_ICASE) && preg->translate == NULL)
> +  if (dfa->is_utf8 && !(syntax & RE_ICASE) && preg->translate == NULL)
>      optimize_utf8 (dfa);
>  #endif
>  
> 
> In our case is_utf8 is 1 and mb_cur_max is also 1. The function
> optimize_utf8() would change "." to match utf8 characters instead of
> bytes. For some reason I have not investigated further then "©" (or any
> other non-ASCII) character is not matched, but in the C locale we want
> "." also to match non-valid utf8 characters anyway.

I think this fix is misplaced; it looks like it would make GNU regex
do UTF-8 character matching rather than byte matching in the C locale.
Rather one of the other places that has an is_utf8 check also needs to
have the mb_cur_max!=1 check added, I think.

Rich


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Re: First feedback on new C locale problems
  2015-09-27 13:47     ` Rich Felker
@ 2015-09-27 13:49       ` Felix Janda
  2015-09-27 16:59         ` Rich Felker
  0 siblings, 1 reply; 11+ messages in thread
From: Felix Janda @ 2015-09-27 13:49 UTC (permalink / raw)
  To: musl

Rich Felker wrote:
> On Sun, Sep 27, 2015 at 08:17:38AM +0200, Felix Janda wrote:
> > Rich Felker wrote:
> > > On Sat, Sep 26, 2015 at 06:58:36AM +0200, Felix Janda wrote:
> > > > On 2015-09-09 05:56:48 GMT, Rich Felker wrote:
> > > > > On Tue, Sep 01, 2015 at 02:32:35AM -0400, Rich Felker wrote:
> > > > > > What I'd like to do to fix it is just always return "UTF-8" for
> > > > > > nl_langinfo(CODESET) regardless of locale (rather than returning
> > > > > > "UTF-8-CODE-UNITS" when in C locale). POSIX places no requirements on
> > > > > > nl_langinfo that would preclude this, and it seems like it would
> > > > > > restore the desired properties and fix all the regressions.
> > > > >
> > > > > Committed.
> > > > >
> > > > > Rich
> > > > 
> > > > GNU sed seems to care about the output from nl_langinfo:
> > > > 
> > > > https://bugs.gentoo.org/show_bug.cgi?id=560728
> > > > 
> > > > More specifically, so does lib/localecharset.c, which is used in
> > > > the replacement of re_compile_pattern.
> > > 
> > > I was able to reproduce this (with slightly different output, "a© a'")
> > > on Alpine. Clearly this is some sort of bug in the gnulib code or sed
> > > itself, since it's producing corrupt output. I think we should explore
> > > why that's happening and whether it's possible to fix there. But if
> > > there remain other reasons that returning "UTF-8" in the C locale is
> > > not practical then perhaps we could resort to returning "ASCII".
> > 
> > A possible fix is
> > 
> > --- ./a/sed-4.2.1/lib/regcomp.c
> > +++ ./a/sed-4.2.1/lib/regcomp.c
> > @@ -824,7 +824,7 @@ re_compile_internal (regex_t *preg, cons
> >  
> >  #ifdef RE_ENABLE_I18N
> >    /* If possible, do searching in single byte encoding to speed things up.  */
> > -  if (dfa->is_utf8 && dfa->mb_cur_max != 1 && !(syntax & RE_ICASE) && preg->translate == NULL)
> > +  if (dfa->is_utf8 && !(syntax & RE_ICASE) && preg->translate == NULL)
> >      optimize_utf8 (dfa);
> >  #endif
> >  
> > 
> > In our case is_utf8 is 1 and mb_cur_max is also 1. The function
> > optimize_utf8() would change "." to match utf8 characters instead of
> > bytes. For some reason I have not investigated further then "©" (or any
> > other non-ASCII) character is not matched, but in the C locale we want
> > "." also to match non-valid utf8 characters anyway.
> 
> I think this fix is misplaced; it looks like it would make GNU regex
> do UTF-8 character matching rather than byte matching in the C locale.
> Rather one of the other places that has an is_utf8 check also needs to
> have the mb_cur_max!=1 check added, I think.

Oh, sorry for the confusion. The patch is inverted...

Felix


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Re: First feedback on new C locale problems
  2015-09-27 13:49       ` Felix Janda
@ 2015-09-27 16:59         ` Rich Felker
  2015-09-28 18:58           ` Rich Felker
  0 siblings, 1 reply; 11+ messages in thread
From: Rich Felker @ 2015-09-27 16:59 UTC (permalink / raw)
  To: musl

On Sun, Sep 27, 2015 at 03:49:02PM +0200, Felix Janda wrote:
> Rich Felker wrote:
> > On Sun, Sep 27, 2015 at 08:17:38AM +0200, Felix Janda wrote:
> > > Rich Felker wrote:
> > > > On Sat, Sep 26, 2015 at 06:58:36AM +0200, Felix Janda wrote:
> > > > > On 2015-09-09 05:56:48 GMT, Rich Felker wrote:
> > > > > > On Tue, Sep 01, 2015 at 02:32:35AM -0400, Rich Felker wrote:
> > > > > > > What I'd like to do to fix it is just always return "UTF-8" for
> > > > > > > nl_langinfo(CODESET) regardless of locale (rather than returning
> > > > > > > "UTF-8-CODE-UNITS" when in C locale). POSIX places no requirements on
> > > > > > > nl_langinfo that would preclude this, and it seems like it would
> > > > > > > restore the desired properties and fix all the regressions.
> > > > > >
> > > > > > Committed.
> > > > > >
> > > > > > Rich
> > > > > 
> > > > > GNU sed seems to care about the output from nl_langinfo:
> > > > > 
> > > > > https://bugs.gentoo.org/show_bug.cgi?id=560728
> > > > > 
> > > > > More specifically, so does lib/localecharset.c, which is used in
> > > > > the replacement of re_compile_pattern.
> > > > 
> > > > I was able to reproduce this (with slightly different output, "a© a'")
> > > > on Alpine. Clearly this is some sort of bug in the gnulib code or sed
> > > > itself, since it's producing corrupt output. I think we should explore
> > > > why that's happening and whether it's possible to fix there. But if
> > > > there remain other reasons that returning "UTF-8" in the C locale is
> > > > not practical then perhaps we could resort to returning "ASCII".
> > > 
> > > A possible fix is
> > > 
> > > --- ./a/sed-4.2.1/lib/regcomp.c
> > > +++ ./a/sed-4.2.1/lib/regcomp.c
> > > @@ -824,7 +824,7 @@ re_compile_internal (regex_t *preg, cons
> > >  
> > >  #ifdef RE_ENABLE_I18N
> > >    /* If possible, do searching in single byte encoding to speed things up.  */
> > > -  if (dfa->is_utf8 && dfa->mb_cur_max != 1 && !(syntax & RE_ICASE) && preg->translate == NULL)
> > > +  if (dfa->is_utf8 && !(syntax & RE_ICASE) && preg->translate == NULL)
> > >      optimize_utf8 (dfa);
> > >  #endif
> > >  
> > > 
> > > In our case is_utf8 is 1 and mb_cur_max is also 1. The function
> > > optimize_utf8() would change "." to match utf8 characters instead of
> > > bytes. For some reason I have not investigated further then "©" (or any
> > > other non-ASCII) character is not matched, but in the C locale we want
> > > "." also to match non-valid utf8 characters anyway.
> > 
> > I think this fix is misplaced; it looks like it would make GNU regex
> > do UTF-8 character matching rather than byte matching in the C locale.
> > Rather one of the other places that has an is_utf8 check also needs to
> > have the mb_cur_max!=1 check added, I think.
> 
> Oh, sorry for the confusion. The patch is inverted...

Ah, ok. But in that case, it's probably best not to detect is_utf8 to
begin with if MB_CUR_MAX==1.

I should probably read the code and try to get a better understanding
of what it's doing.

Rich


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Re: First feedback on new C locale problems
  2015-09-27 16:59         ` Rich Felker
@ 2015-09-28 18:58           ` Rich Felker
  2015-09-29  4:00             ` Felix Janda
  0 siblings, 1 reply; 11+ messages in thread
From: Rich Felker @ 2015-09-28 18:58 UTC (permalink / raw)
  To: musl

On Sun, Sep 27, 2015 at 12:59:25PM -0400, Rich Felker wrote:
> On Sun, Sep 27, 2015 at 03:49:02PM +0200, Felix Janda wrote:
> > Rich Felker wrote:
> > > On Sun, Sep 27, 2015 at 08:17:38AM +0200, Felix Janda wrote:
> > > > Rich Felker wrote:
> > > > > On Sat, Sep 26, 2015 at 06:58:36AM +0200, Felix Janda wrote:
> > > > > > On 2015-09-09 05:56:48 GMT, Rich Felker wrote:
> > > > > > > On Tue, Sep 01, 2015 at 02:32:35AM -0400, Rich Felker wrote:
> > > > > > > > What I'd like to do to fix it is just always return "UTF-8" for
> > > > > > > > nl_langinfo(CODESET) regardless of locale (rather than returning
> > > > > > > > "UTF-8-CODE-UNITS" when in C locale). POSIX places no requirements on
> > > > > > > > nl_langinfo that would preclude this, and it seems like it would
> > > > > > > > restore the desired properties and fix all the regressions.
> > > > > > >
> > > > > > > Committed.
> > > > > > >
> > > > > > > Rich
> > > > > > 
> > > > > > GNU sed seems to care about the output from nl_langinfo:
> > > > > > 
> > > > > > https://bugs.gentoo.org/show_bug.cgi?id=560728
> > > > > > 
> > > > > > More specifically, so does lib/localecharset.c, which is used in
> > > > > > the replacement of re_compile_pattern.
> > > > > 
> > > > > I was able to reproduce this (with slightly different output, "a© a'")
> > > > > on Alpine. Clearly this is some sort of bug in the gnulib code or sed
> > > > > itself, since it's producing corrupt output. I think we should explore
> > > > > why that's happening and whether it's possible to fix there. But if
> > > > > there remain other reasons that returning "UTF-8" in the C locale is
> > > > > not practical then perhaps we could resort to returning "ASCII".
> > > > 
> > > > A possible fix is
> > > > 
> > > > --- ./a/sed-4.2.1/lib/regcomp.c
> > > > +++ ./a/sed-4.2.1/lib/regcomp.c
> > > > @@ -824,7 +824,7 @@ re_compile_internal (regex_t *preg, cons
> > > >  
> > > >  #ifdef RE_ENABLE_I18N
> > > >    /* If possible, do searching in single byte encoding to speed things up.  */
> > > > -  if (dfa->is_utf8 && dfa->mb_cur_max != 1 && !(syntax & RE_ICASE) && preg->translate == NULL)
> > > > +  if (dfa->is_utf8 && !(syntax & RE_ICASE) && preg->translate == NULL)
> > > >      optimize_utf8 (dfa);
> > > >  #endif
> > > >  
> > > > 
> > > > In our case is_utf8 is 1 and mb_cur_max is also 1. The function
> > > > optimize_utf8() would change "." to match utf8 characters instead of
> > > > bytes. For some reason I have not investigated further then "©" (or any
> > > > other non-ASCII) character is not matched, but in the C locale we want
> > > > "." also to match non-valid utf8 characters anyway.
> > > 
> > > I think this fix is misplaced; it looks like it would make GNU regex
> > > do UTF-8 character matching rather than byte matching in the C locale.
> > > Rather one of the other places that has an is_utf8 check also needs to
> > > have the mb_cur_max!=1 check added, I think.
> > 
> > Oh, sorry for the confusion. The patch is inverted...
> 
> Ah, ok. But in that case, it's probably best not to detect is_utf8 to
> begin with if MB_CUR_MAX==1.
> 
> I should probably read the code and try to get a better understanding
> of what it's doing.

I think the actual error is here:

http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/regcomp.c#n903

In the _LIBC code path, they check MB_CUR_LEN==6 (glibc's nonstandard
value they use for UTF-8) perhaps just as an optimization of the
non-UTF-8 case, but they don't check it for !_LIBC; they just rely on
the CODESET name matching.

I'm still somewhat concerned that returning "UTF-8" is problematic
here, but I think gnulib also has a bug; trusting their interpretation
of the string returned by nl_langinfo(CODESET) seems to be leading to
corrupt results.

Rich


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Re: First feedback on new C locale problems
  2015-09-28 18:58           ` Rich Felker
@ 2015-09-29  4:00             ` Felix Janda
  0 siblings, 0 replies; 11+ messages in thread
From: Felix Janda @ 2015-09-29  4:00 UTC (permalink / raw)
  To: musl

Rich Felker wrote:
> On Sun, Sep 27, 2015 at 12:59:25PM -0400, Rich Felker wrote:
> > On Sun, Sep 27, 2015 at 03:49:02PM +0200, Felix Janda wrote:
> > > Rich Felker wrote:
> > > > On Sun, Sep 27, 2015 at 08:17:38AM +0200, Felix Janda wrote:
> > > > > Rich Felker wrote:
> > > > > > On Sat, Sep 26, 2015 at 06:58:36AM +0200, Felix Janda wrote:
> > > > > > > On 2015-09-09 05:56:48 GMT, Rich Felker wrote:
> > > > > > > > On Tue, Sep 01, 2015 at 02:32:35AM -0400, Rich Felker wrote:
> > > > > > > > > What I'd like to do to fix it is just always return "UTF-8" for
> > > > > > > > > nl_langinfo(CODESET) regardless of locale (rather than returning
> > > > > > > > > "UTF-8-CODE-UNITS" when in C locale). POSIX places no requirements on
> > > > > > > > > nl_langinfo that would preclude this, and it seems like it would
> > > > > > > > > restore the desired properties and fix all the regressions.
> > > > > > > >
> > > > > > > > Committed.
> > > > > > > >
> > > > > > > > Rich
> > > > > > > 
> > > > > > > GNU sed seems to care about the output from nl_langinfo:
> > > > > > > 
> > > > > > > https://bugs.gentoo.org/show_bug.cgi?id=560728
> > > > > > > 
> > > > > > > More specifically, so does lib/localecharset.c, which is used in
> > > > > > > the replacement of re_compile_pattern.
> > > > > > 
> > > > > > I was able to reproduce this (with slightly different output, "a© a'")
> > > > > > on Alpine. Clearly this is some sort of bug in the gnulib code or sed
> > > > > > itself, since it's producing corrupt output. I think we should explore
> > > > > > why that's happening and whether it's possible to fix there. But if
> > > > > > there remain other reasons that returning "UTF-8" in the C locale is
> > > > > > not practical then perhaps we could resort to returning "ASCII".
> > > > > 
> > > > > A possible fix is
> > > > > 
> > > > > --- ./a/sed-4.2.1/lib/regcomp.c
> > > > > +++ ./a/sed-4.2.1/lib/regcomp.c
> > > > > @@ -824,7 +824,7 @@ re_compile_internal (regex_t *preg, cons
> > > > >  
> > > > >  #ifdef RE_ENABLE_I18N
> > > > >    /* If possible, do searching in single byte encoding to speed things up.  */
> > > > > -  if (dfa->is_utf8 && dfa->mb_cur_max != 1 && !(syntax & RE_ICASE) && preg->translate == NULL)
> > > > > +  if (dfa->is_utf8 && !(syntax & RE_ICASE) && preg->translate == NULL)
> > > > >      optimize_utf8 (dfa);
> > > > >  #endif
> > > > >  
> > > > > 
> > > > > In our case is_utf8 is 1 and mb_cur_max is also 1. The function
> > > > > optimize_utf8() would change "." to match utf8 characters instead of
> > > > > bytes. For some reason I have not investigated further then "©" (or any
> > > > > other non-ASCII) character is not matched, but in the C locale we want
> > > > > "." also to match non-valid utf8 characters anyway.
> > > > 
> > > > I think this fix is misplaced; it looks like it would make GNU regex
> > > > do UTF-8 character matching rather than byte matching in the C locale.
> > > > Rather one of the other places that has an is_utf8 check also needs to
> > > > have the mb_cur_max!=1 check added, I think.
> > > 
> > > Oh, sorry for the confusion. The patch is inverted...
> > 
> > Ah, ok. But in that case, it's probably best not to detect is_utf8 to
> > begin with if MB_CUR_MAX==1.
> > 
> > I should probably read the code and try to get a better understanding
> > of what it's doing.
> 
> I think the actual error is here:
> 
> http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/regcomp.c#n903
> 
> In the _LIBC code path, they check MB_CUR_LEN==6 (glibc's nonstandard
> value they use for UTF-8) perhaps just as an optimization of the
> non-UTF-8 case, but they don't check it for !_LIBC; they just rely on
> the CODESET name matching.

Upon your previous mail I had come to the same conclusions. Maybe they
would not be opposed to optimizing the non-UTF-8 when !_LIBC using
MB_CUR_MAX.

Unfortunately, the GNU regex code seems to be copied into quite a lot
of projects.

Felix

> I'm still somewhat concerned that returning "UTF-8" is problematic
> here, but I think gnulib also has a bug; trusting their interpretation
> of the string returned by nl_langinfo(CODESET) seems to be leading to
> corrupt results.
>
> Rich


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: First feedback on new C locale problems
  2015-09-01  6:32 Rich Felker
  2015-09-09  5:56 ` Rich Felker
@ 2015-09-28 19:34 ` Rich Felker
  1 sibling, 0 replies; 11+ messages in thread
From: Rich Felker @ 2015-09-28 19:34 UTC (permalink / raw)
  To: musl

I'm revisiting this thread because returning "UTF-8" for
nl_langinfo(CODESET) in the C locale has seriously broken software
using GNU regex (e.g. GNU sed) and seems unsafe in general. What's
happening is that part of the code is using mbrtowc, and part is doing
its own UTF-8 handling based on concluding from nl_langinfo(CODESET)
that the locale is UTF-8 based. While there's no actual requirement in
the standard for this to work, it seems reasonable for applications to
expect that it works, and it presumably works on all existing
implementations including all releases of musl (breakage is from
commit 844212d94f582c4e3c5055e0a1524931e89ebe76, not yet in a
release), and I'd really rather not make the real-world situation for
handling UTF-8 _worse_ for applications.

On Tue, Sep 01, 2015 at 02:32:35AM -0400, Rich Felker wrote:
> So far I've gotten 2 reports of things breaking from the new C locale.
> The first was in Alpine:
> 
> gpg-connect-agent: conversion from 'utf-8' to 'UTF-8-CODE-UNITS' not available
> 
> and turned out to be caused by --disable-nls omitting setlocale,
> leading to nl_langinfo(CODESET) requesting the C locale's codeset
> name. This could be fixed by making iconv support "UTF-8-CODE-UNITS"
> and do something reasonable with it, but the second issue was uglier.

Alpine now has a patch to call setlocale even when --disable-nls is
used, and I think this is the right behavior. It's unreasonable to
ever call nl_langinfo(CODESET) if you don't call setlocale or
newlocale/uselocale. This patch should probably be upstreamed.
Alternatively, they could remove the iconv code and pass strings
through without any conversion when NLS is disabled, but that's
probably a bad idea.

> In Void Linux:
> 
> help2man:
> Unknown encoding 'UTF-8-CODE-UNITS' at /usr/bin/help2man line 56.
> (https://github.com/voidlinux/void-packages/issues/2425)
> 
> Oddly this one did not affect Alpine, for the same reason the first
> one did: Alpine has gettext support turned off, and help2man omits the
> offending code:
> 
> http://anonscm.debian.org/cgit/users/bod/help2man.git/tree/help2man.PL?id=9ce0caa4cf164261ddde3fe987a260f5ba0dd558#n117
> 
> (which is overriding the system locale with "C" by default) when
> gettext support is disabled. Being that this is Perl code and it's
> passing the charset name to Perl's conversion functions, we can't just
> work around this by adding a new charset alias to iconv.
> 
> Note that fixing the broken programs to call setlocale properly and
> honor the user's locale would make them work in the normal case, but
> they would break again if the user explicitly invoked them with
> LC_CTYPE=C.

This remains the case. For all practical purposes,
nl_langinfo(CODESET) must return a string which is a "well-known"
character encoding name. This means there are exactly two choices:
"UTF-8" or "ASCII".

Neither is ideal. Returning "UTF-8" to the application misrepresents
that multibyte character processing is active, and we've seen actual
breakage (GNU regex) with this. GNU regex could be fixed by also
checking MB_CUR_MAX here:

http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/regcomp.c#n903

Actually they already do that in the (g)libc-internal code path, but
not as part of gnulib. But in general, I'm worried that it doesn't
make sense to demand that applications do this (and essentially create
a situation where "UTF-8" can have two different meanings, i.e. "UTF-8
multibyte characters" and "nominal UTF-8 processed in units of
bytes").

The other option, "ASCII", is also imperfect but perhaps better. The
only place it's inconsistent is that iconv with "ASCII" as the
in_charset would give EILSEQ for high bytes whereas mbrtowc would
accept them and successfully round-trip them. But in conveying to
applications the sense of "you're intentionally using a restricted
character-set environment and ASCII is all that you can meaningfully
use", it's accurate. It certainly doesn't permit any erroneous usage
or misinterpretation of data.

> Anyway, what I suspect is that we're going to find a fair number of
> programs are calling nl_langinfo(CODESET) without actually having set
> the locale properly. As long as they're not using multibyte functions
> in libc to process text, failing to have called setlocale is not such
> a bad thing; they can do character processing themselves if they know
> the intended encoding, using iconv or native UTF-8 code or whatever.
> And the situation we've got right now is that, despite best efforts
> not to impact users who don't intentionally _try_ to get a byte-based
> C locale, this functionality is causing actual regressions in musl's
> promise of "always UTF-8".

I suspect this problem will resurface, but at least we can find the
affected applications and get them fixed. Does this sound reasonable?

Rich


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: First feedback on new C locale problems
  2015-09-01  6:32 Rich Felker
@ 2015-09-09  5:56 ` Rich Felker
  2015-09-28 19:34 ` Rich Felker
  1 sibling, 0 replies; 11+ messages in thread
From: Rich Felker @ 2015-09-09  5:56 UTC (permalink / raw)
  To: musl

On Tue, Sep 01, 2015 at 02:32:35AM -0400, Rich Felker wrote:
> What I'd like to do to fix it is just always return "UTF-8" for
> nl_langinfo(CODESET) regardless of locale (rather than returning
> "UTF-8-CODE-UNITS" when in C locale). POSIX places no requirements on
> nl_langinfo that would preclude this, and it seems like it would
> restore the desired properties and fix all the regressions.

Committed.

Rich


^ permalink raw reply	[flat|nested] 11+ messages in thread

* First feedback on new C locale problems
@ 2015-09-01  6:32 Rich Felker
  2015-09-09  5:56 ` Rich Felker
  2015-09-28 19:34 ` Rich Felker
  0 siblings, 2 replies; 11+ messages in thread
From: Rich Felker @ 2015-09-01  6:32 UTC (permalink / raw)
  To: musl

So far I've gotten 2 reports of things breaking from the new C locale.
The first was in Alpine:

gpg-connect-agent: conversion from 'utf-8' to 'UTF-8-CODE-UNITS' not available

and turned out to be caused by --disable-nls omitting setlocale,
leading to nl_langinfo(CODESET) requesting the C locale's codeset
name. This could be fixed by making iconv support "UTF-8-CODE-UNITS"
and do something reasonable with it, but the second issue was uglier.
In Void Linux:

help2man:
Unknown encoding 'UTF-8-CODE-UNITS' at /usr/bin/help2man line 56.
(https://github.com/voidlinux/void-packages/issues/2425)

Oddly this one did not affect Alpine, for the same reason the first
one did: Alpine has gettext support turned off, and help2man omits the
offending code:

http://anonscm.debian.org/cgit/users/bod/help2man.git/tree/help2man.PL?id=9ce0caa4cf164261ddde3fe987a260f5ba0dd558#n117

(which is overriding the system locale with "C" by default) when
gettext support is disabled. Being that this is Perl code and it's
passing the charset name to Perl's conversion functions, we can't just
work around this by adding a new charset alias to iconv.

Note that fixing the broken programs to call setlocale properly and
honor the user's locale would make them work in the normal case, but
they would break again if the user explicitly invoked them with
LC_CTYPE=C.

Anyway, what I suspect is that we're going to find a fair number of
programs are calling nl_langinfo(CODESET) without actually having set
the locale properly. As long as they're not using multibyte functions
in libc to process text, failing to have called setlocale is not such
a bad thing; they can do character processing themselves if they know
the intended encoding, using iconv or native UTF-8 code or whatever.
And the situation we've got right now is that, despite best efforts
not to impact users who don't intentionally _try_ to get a byte-based
C locale, this functionality is causing actual regressions in musl's
promise of "always UTF-8".

What I'd like to do to fix it is just always return "UTF-8" for
nl_langinfo(CODESET) regardless of locale (rather than returning
"UTF-8-CODE-UNITS" when in C locale). POSIX places no requirements on
nl_langinfo that would preclude this, and it seems like it would
restore the desired properties and fix all the regressions.

I don't mind leaving it as-is for a little bit while we discuss this
though; hopefully we'll turn up some more interesting application bugs
to get fixed.

Rich


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2015-09-29  4:00 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-26  4:58 First feedback on new C locale problems Felix Janda
2015-09-26 19:35 ` Rich Felker
2015-09-27  6:17   ` Felix Janda
2015-09-27 13:47     ` Rich Felker
2015-09-27 13:49       ` Felix Janda
2015-09-27 16:59         ` Rich Felker
2015-09-28 18:58           ` Rich Felker
2015-09-29  4:00             ` Felix Janda
  -- strict thread matches above, loose matches on Subject: below --
2015-09-01  6:32 Rich Felker
2015-09-09  5:56 ` Rich Felker
2015-09-28 19:34 ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).