[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20261026212846.GG23438@brightrain.aerifal.cx>
Date: Mon, 26 Oct 2026 17:28:46 -0400
From: Rich Felker <dalias@...c.org>
To: Luca Kellermann <mailto.luca.kellermann@...il.com>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH v2] musl locale overhaul series, take 2
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:
> > [...]
> > > > if (item == CODESET) return loc->cat[LC_CTYPE] ? "UTF-8" : "ASCII";
> > > > + //if (item == RADIXCHAR && loc->cat[LC_NUMERIC])
> > > > + // return loc->cat[LC_NUMERIC]->lconv.decimal_point;
> > >
> > > RADIXCHAR and THOUSEP could be implemented like this to avoid
> > > redundancy in the file format:
> >
> > They actually can't because lconv is not present in the __locale_map
> > structure. It is derived from both LC_NUMERIC and LC_MONETARY combined
> > and thus is only present in the __locale_struct, and only if
> > localeconv is linked/used.
>
> With "implemented like this" I meant the following code, not what was
> commented out.
>
> > It would be possible to just dynamically lookup the lconv strings here
> > rather than using the langinfo table, but that's more code (linked in
> > every static binary, and cognitive burden to understand) which seems
> > like a bad tradeoff for a few bytes of automatically generate locale
> > data.
>
> That's exactly what the code below does.
>
> > > if (item == RADIXCHAR) {
> > > static const int r_path[] = LPATH(K_LCONV, 0);
> > > const char *str = __ikmlt_lookup_path(
> > > LMEMB(loc, LC_NUMERIC, table_root), r_path);
> > > if (!str || str[0] != ',' || str[1]) str = ".";
> > > return (char *)str;
> > > }
> > > if (item == THOUSEP) {
> > > static const int t_path[] = LPATH(K_LCONV, 1);
> > > const char *str = __ikmlt_lookup_path(
> > > LMEMB(loc, LC_NUMERIC, table_root), t_path);
> > > return str ? (char *)str : "";
> > > }
> > >
> > > lc_numeric.h could then be removed and LC_TIME (2) could be used as
> > > start in c_langinfo.
> >
> > Saving 4 bytes of table by spending ~100 bytes of code?
>
> I suggested this because it would make it impossible to have
> mismatched values for RADIXCHAR/decimal_point or THOUSEP/thousands_sep
> (e.g. due to translator mistake).
>
> 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.
Of course with intentionally-bad tooling you could make misaligned
values, but with intentionally-bad tooling you could also make table
structures that point outside the map, etc.
Locale files are assumed to be trustworthy and are not loaded from
user-controlled locations in the case of suids etc.
> > > Is it possible to synthesize CRNCYSTR from currency_symbol,
> > > p_cs_precedes and n_cs_precedes (or the other way around)? If so, I
> > > have some ideas how to get rid of it from the file format.
> >
> > Yes but then you need writable runtime storage for it. Read-only
> > mappings are free. Trading a few bytes of read-only mappings for
> > read-write buffers per-process and significantly more code to generate
> > the data at runtime is a very bad tradeoff in the opposite direction
> > musl generally takes.
>
> Better do this in the tooling then if deemed useful.
Yes, it also does this.
> > > > /* _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.
> > > > From 27359091d7126f0edaa30796eb102596446eacdb Mon Sep 17 00:00:00 2001
> > > > From: Rich Felker <dalias@...ifal.cx>
> > > > Date: Mon, 14 Sep 2026 18:09:27 -0400
> > > > Subject: [PATCH 09/23] locale: make explicit request of unavailable locales
> > > > fail
> > > >
> > > > have __get_locale track whether the request is implicit (obtained via
> > > > environment for an argument of "") or explicit. if explicit, names
> > > > that fail to resove to a locale file will produce a return value of
> > > > LOC_MAP_FAILED, which setlocale and newlocale already interpret as an
> > > > error condition. if implicit, return C.UTF-8 for LC_CTYPE, and
> > > > otherwise a null pointer (interpreted as C locale).
> > >
> > > setlocale(LC_ALL, "some-very-very-very-very-very-very-long-name") will
> > > still not fail but behave as setlocale(LC_ALL, "C.UTF-8"). setlocale()
> > > could be changed like this:
> >
> > Why do you think that will happen? If the loop that checks the name
> > length and characters present ends early, val[n] is nonzero and we
> > take the goto fail. If !implicit, this returns LOC_MAP_FAILED. Only
> > for setlocale(LC_ALL /* or LC_CTYPE */, "") with the LC_* vars set to
> > something invalid will you get C.UTF_8.
>
> The problem is in setlocale.c, not locale_map.c:
>
> char part[LOCALE_NAME_MAX+1] = "C.UTF-8";
> const char *p = name;
> for (i=0; i<LC_ALL; i++) {
> const char *z = __strchrnul(p, ';');
> if (z-p <= LOCALE_NAME_MAX) {
> memcpy(part, p, z-p);
> part[z-p] = 0;
> if (*z) p = z+1;
> }
> // for long names, part is still "C.UTF-8" at this point
> lm = __get_locale(i, part);
> // ...
> }
>
> > > char *setlocale(int cat, const char *name)
> > > {
> > > // ...
> > > if (cat == LC_ALL) {
> > > int i;
> > > if (name) {
> > > struct __locale_struct tmp_locale;
> > > const char *p = name;
> > > for (i=0; i<LC_ALL; i++) {
> > > const char *z = __strchrnul(p, ';');
> > > if (z-p > LOCALE_NAME_MAX) {
> > > UNLOCK(__locale_lock);
> > > return 0;
> > > }
> > > char part[LOCALE_NAME_MAX+1];
> > > memcpy(part, p, z-p);
> > > part[z-p] = 0;
> > > if (*z) p = z+1;
> > > lm = __get_locale(i, part);
> > > // ...
> > > }
> > > // ...
> > > }
> > > // ...
> > > }
> > > // ...
> > > }
> >
> > We already have an error-out path for LOC_MAP_FAILED.
>
> This is the fix for the problem described above.
Oh, yes, I see it now. And this code path is always using the explicit
argument name not the env vars, so it's always fine to error out on
long names.
> > > > +static int load_locale(struct __locale_map *l, const unsigned char *lm)
> > > > +{
> > > > + const unsigned char *root;
> > > > +
> > > > + if (memcmp(lm, "ikmlt\xff\xc0\xc1loc2\0\0\0\0", 16)) {
> > >
> > > Does it make sense to ensure that map_size > (16 + 8) (magic + header
> > > of root table) before?
>
> What do you think about this? As in:
>
> const void *map = __map_file(buf, &map_size);
> // ...
> if (map_size <= 24) goto unmap; // skip load_locale()
>
> Or is it always valid to access these first few bytes?
Unless the map is zero-length, in practice it's non-faulting to access
within the same page past the end of the file.
I'm not sure how much error checking we want to have for things like
this, but it wouldn't hurt to do a simple length check like that.
We could make all lookups check against the end of the mapping, but
it's extra work in the hot paths (and exposing the end pointer to the
collation backends, etc.) that would only be for the sake of not
crashing on malformed locale files. That level of checking probably
does not make sense.
> > > > - 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.
> > > > From aba708364983906ee36b1842fb1dc62dd157eefd Mon Sep 17 00:00:00 2001
> > > > From: Rich Felker <dalias@...ifal.cx>
> > > > Date: Fri, 18 Sep 2026 12:08:29 -0400
> > > > Subject: [PATCH 17/23] add collation implementation
> > [...]
> > > > diff --git a/src/locale/locale_map.c b/src/locale/locale_map.c
> > > > [...]
> > > > + collation_init(l->collate, ikmlt_lookup(root, K_COLLATION));
> > >
> > > Why does collation_init() return a value if it's unused?
> >
> > We might end up using it, or not. At present libc just doesn't need
> > the return value because it's expected to setup a valid
> > no-collation-data state if it fails, and that's what we want on
> > failure.
>
> Also makes sense when using collate.c outside of musl.
Yep.
> > > > From 304767d2576e1c285c76afb71522bd5bbdad85ed Mon Sep 17 00:00:00 2001
> > > > From: Rich Felker <dalias@...ifal.cx>
> > > > Date: Thu, 17 Sep 2026 14:11:48 -0400
> > > > Subject: [PATCH 18/23] move global_locale object out of libc struct
> > > >
> > > > [...]
> > > >
> > > > diff --git a/src/internal/libc.h b/src/internal/libc.h
> > > > [...]
> > > > +extern hidden struct __locale_struct __global_locale;
> > >
> > > This could be declared in locale_impl.h instead. locale_impl.h would
> > > then no longer need to include libc.h.
> >
> > Maybe a good idea, but I think it would require other files to include
> > locale_impl.h. Files where it would be somewhat more invasive and
> > unexpected. So keeping it in libc.h is probably better.
>
> Yes, just __init_tls.c and pthread_create.c though. These already
> change with this commit.
By invasive I didn't mean the size of the diff, but bringing any
macros etc. from locale_impl.h into non-locale-related files.
>
> > > > 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.
> > > > 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.
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.
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.
