[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <auO5W6lNDVn0RxjH@mail.gmail.com>
Date: Thu, 29 Oct 2026 22:23:39 +0100
From: Luca Kellermann <mailto.luca.kellermann@...il.com>
To: Rich Felker <dalias@...c.org>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH v2] musl locale overhaul series, take 2
On Mon, Oct 26, 2026 at 05:28:46PM -0400, Rich Felker wrote:
> On Mon, Oct 26, 2026 at 05:39:52PM +0100, Luca Kellermann wrote:
> > On Sat, Oct 24, 2026 at 05:52:14PM -0400, Rich Felker wrote:
> [...]
> > But the tooling could also ensure this. Only allow one of each
> > RADIXCHAR/decimal_point and THOUSEP/thousands_sep in the source format
> > and use it for both in the generated file.
>
> This is exactly what the tooling does now. There's no need to manually
> align the values.
Nice. I guess I should have checked this before.
> [...]
> > > > > /* _NL_LOCALE_NAME extension */
> > > > > - if (idx == 65535 && cat < LC_ALL)
> > > > > + if (idx == 65535 && (unsigned)cat < LC_ALL)
> > > >
> > > > Instead of casting to unsigned here, maybe cast item to unsigned
> > > > before shifting (shifting negative values is implementation-defined).
> > >
> > > Yes, that's code that was already present tho and not really a big
> > > deal. Either definition is fine here.
> > >
> > > > > [...]
> > > > > + const char *str = __loc_lookup((const void *)&c_langinfo,
> > > > > + 0 /* LMEMB(CURRENT_LOCALE,cat,langinfo) */,
> > > > > + path, unknown);
> > > >
> > > > This should use loc instead of CURRENT_LOCALE. There will also be an
> > > > out of bounds access to loc->cat if cat < 0 or cat >= LC_ALL.
> > >
> > > Thanks! That's a real bug.
> >
> > A fix for both the implementation-defined shift behavior and the out
> > of bounds access:
> >
> > char *__nl_langinfo_l(nl_item item, locale_t loc)
> > {
> > int cat = ((unsigned)item) >> 16;
> > if (cat >= LC_ALL) return "";
> > // ...
> > if (idx == 65535)
> > return loc->cat[cat] ? (char *)loc->cat[cat]->name : "C";
> > // ...
> > }
>
> This is a little worse I think because it moves the check for an
> uncommon erorr condition (programmer error) to a place where it's
> executed even in code paths where it's not needed.
Yes, the check for cat >= LC_ALL could be moved below CODESET but it's
needed before _NL_LOCALE_NAME and LMEMB().
> [...]
> > > > > - new->map = map;
> > > > > - new->map_size = map_size;
> > > > > + *new = tmp;
> > > > > memcpy(new->name, val, n);
> > > > > new->name[n] = 0;
> > > > > new->next = loc_head;
> > > >
> > > > Writing it this way would avoid copying a partially uninitialized tmp
> > > > (even when dummy_collation_init() is used later):
> > > >
> > > > struct __locale_map tmp = { 0 };
> > > > if (load_locale(&tmp, map) ||
> > > > !(new = malloc(sizeof *new))) {
> > > > // ...
> > > > break;
> > > > }
> > > > memcpy(tmp.name, val, n);
> > > > tmp.name[n] = 0;
> > > > tmp.next = loc_head;
> > > > *new = tmp;
> > > > loc_head = new;
> > >
> > > I guess something like this could be done. Is there a motivation for
> > > avoiding the uninitialized contents that won't be used?
> >
> > I just wasn't sure if copying uninitialized memory like this is some
> > sort of UB, especially if pointers are involved.
>
> Assignment of structs is well-defined regardless of whether any
> members are uninitialized or indeterminate.
Do you know where this is specified in the standard? I was only able
to find this (listed under J.2 Undefined behavior): "The value of an
object with automatic storage duration is used while it is
indeterminate"
> [...]
> > > > > From c45dbe1d43d5a5aab85f529902f3f57d8149fe7a Mon Sep 17 00:00:00 2001
> > > > > From: Rich Felker <dalias@...ifal.cx>
> > > > > Date: Fri, 18 Sep 2026 11:32:47 -0400
> > > > > Subject: [PATCH 19/23] implement non-stub localeconv
> > > [...]
> > > > duplocale() could be changed like this:
> > > >
> > > > static void dummy_lconv_expand(locale_t loc)
> > > > {
> > > > }
> > > >
> > > > weak_alias(dummy_lconv_expand, __lconv_expand);
> > > >
> > > > locale_t __duplocale(locale_t old)
> > > > {
> > > > locale_t new = malloc(sizeof *new + sizeof *new->lconv);
> > > > if (!new) return 0;
> > > > if (old == LC_GLOBAL_LOCALE) old = &__global_locale;
> > > > *new = *old;
> > > > new->lconv = (void *)(new + 1);
> > > > __lconv_expand(new);
> > > > return new;
> > > > }
> > >
> > > That works, but I think we can avoid calling __lconv_expand here and
> > > avoid the need for a weak dummy for it. Just copying the contents of
> > > the lconv struct works. Yes it contains pointers but they are all
> > > pointers to immutable string objects.
> > >
> > > So just: *new->lconv = *old->lconv;
> >
> > old->lconv can be a null pointer if old is &__global_locale or
> > &default_ctype_locale.
> >
> > lconv would also be uninitialized if localeconv() isn't linked in, if
> > that's a problem.
>
> OK. This can be made correct, but it seems fragile and nonobvious
> enough that __lconv_expand like you suggested is probably the smarter
> choice.
I came to the same conclusion before my suggestion.
> > > > > From 331f8c00a8dba3da34367983fb05569c7e0bc570 Mon Sep 17 00:00:00 2001
> > > > > From: Rich Felker <dalias@...ifal.cx>
> > > > > Date: Sun, 20 Sep 2026 21:16:22 -0400
> > > > > Subject: [PATCH 20/23] implement a working, non-stub strfmon
> > > > >
> > > > > [...]
> > > > >
> > > > > diff --git a/src/locale/strfmon.c b/src/locale/strfmon.c
> > > > > [...]
> > > > > static ssize_t vstrfmon_l(char *s, size_t n, locale_t loc, const char *fmt, va_list ap)
> > > > > {
> > > > > + locale_t old = __uselocale(loc);
> > > > > + const struct lconv *lc = localeconv();
> > > > > + __uselocale(old);
> > > >
> > > > Other functions that temporarily change the current locale (like
> > > > iconv() or fputwc()) just use CURRENT_LOCALE instead of uselocale().
> > >
> > > I'm fairly indifferent to consistency on things like this. Often
> > > functions which are already written in terms of musl internals use the
> > > internal method, while functions like strfmon that are "mostly or
> > > entirely portable library code" use the public interfaces or
> > > __-prefixed versions thereof.
> >
> > Using __uselocale() is probably better then. Should
> > src/include/locale.h be added to declare __uselocale()?
>
> strfmon is POSIX not base C, so we could just use uselocale (without
> the __ prefix). That's probably cleaner.
Yes.
> I've also thought about getting rid of the _l part in vstrfmon_l and
> moving the uselocale up to strfmon_l. Then there is no uselocale
> cycling in the normal, non-_l strfmon and there are fewer args to
> pass.
Does the current locale matter when calling snprintf() from
vstrfmon_l()? It would change with this suggestion.
> Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
