[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 17 Mar 2017 15:27:49 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: Re: a bug in bindtextdomain() and strip '.UTF-8'
As discussed on IRC, I'm preparing a version that fixes some issues. I
also found a few more problems after you logged off which I'll
describe inline:
On Sat, Mar 04, 2017 at 04:02:58PM +0800, He X wrote:
> --- a/src/locale/dcngettext.c 2017-02-06 14:39:17.860482624 +0000
> +++ b/src/locale/dcngettext.c 2017-02-06 14:39:17.860482624 +0000
> @@ -100,7 +100,9 @@
> size_t map_size;
> void *volatile plural_rule;
> volatile int nplurals;
> - char name[];
> + struct binding *binding;
> + struct __locale_map *lm;
This needs to be const struct __locale_map *lm because the value
assigned to it is pointer-to-const.
> + int cat;
> };
>
> static char *dummy_gettextdomain()
> @@ -120,8 +122,9 @@
> + if (!msgid1) goto notrans;
This overlaps with the other patch so I'll factor it out.
> struct msgcat *p;
> struct __locale_struct *loc = CURRENT_LOCALE;
> const struct __locale_map *lm;
> - const char *dirname, *locname, *catname;
> - size_t dirlen, loclen, catlen, domlen;
> + size_t domlen;
> + struct binding *q;
>
> if ((unsigned)category >= LC_ALL) goto notrans;
>
> @@ -130,47 +132,64 @@
> domlen = strnlen(domainname, NAME_MAX+1);
> if (domlen > NAME_MAX) goto notrans;
>
> - dirname = gettextdir(domainname, &dirlen);
> - if (!dirname) goto notrans;
> + for (q=bindings; q; q=q->next)
> + if (!strcmp(q->domainname, domainname) && q->active)
> + break;
> + if (!q) goto notrans;
>
> lm = loc->cat[category];
> if (!lm) {
> notrans:
> return (char *) ((n == 1) ? msgid1 : msgid2);
> }
> - locname = lm->name;
> -
> - catname = catnames[category];
> - catlen = catlens[category];
> - loclen = strlen(locname);
> -
> - size_t namelen = dirlen+1 + loclen+1 + catlen+1 + domlen+3;
> - char name[namelen+1], *s = name;
> -
> - memcpy(s, dirname, dirlen);
> - s[dirlen] = '/';
> - s += dirlen + 1;
> - memcpy(s, locname, loclen);
> - s[loclen] = '/';
> - s += loclen + 1;
> - memcpy(s, catname, catlen);
> - s[catlen] = '/';
> - s += catlen + 1;
> - memcpy(s, domainname, domlen);
> - s[domlen] = '.';
> - s[domlen+1] = 'm';
> - s[domlen+2] = 'o';
> - s[domlen+3] = 0;
>
> for (p=cats; p; p=p->next)
> - if (!strcmp(p->name, name))
> + if (p->binding == q && p->lm == lm && p->cat == category)
> break;
>
> if (!p) {
> + const char *dirname, *locname, *catname;
> + size_t dirlen, loclen, catlen;
> void *old_cats;
> size_t map_size;
> - const void *map = __map_file(name, &map_size);
> +
> + dirname = q->dirname;
> + locname = lm->name;
> + catname = catnames[category];
> +
> + dirlen = q->dirlen;
> + loclen = strlen(locname);
> + catlen = catlens[category];
> +
> + size_t namelen = dirlen+1 + loclen+1 + catlen+1 + domlen+3;
> + char name[namelen+1];
> + char locbuf[loclen+1], *locp = locbuf;
> + const void *map;
> +
> + memcpy(locbuf, locname, loclen);
> + locbuf[loclen] = 0;
> +
> + for (;;) {
> + snprintf(name, namelen+1, "%s/%s/%s/%s.mo\0", dirname, locbuf, catname, domainname);
> + if (map = __map_file(name, &map_size)) break;
> +
> + if (locp = strchr(locbuf, '.')) {
> + *locp = 0;
As discussed on irc, .charset suffixes should be dropped before the
loop even begins (never used in pathnames), and they occur before the
@mod, not after it, so the logic for dropping them is different.
> + } else if (locp = strchr(locbuf, '@')) {
> + *locp = 0;
> + locbuf[loclen] = '@';
> + } else if (locp = strchr(locbuf, '_')) {
> + if (locbuf[loclen] == '@') {
> + locbuf[loclen] = 0;
> + *locp = '@';
> + strcat(locp+1, locbuf + strlen(locbuf) + 1);
This invocation of strcat is UB because src and dest overlap; you need
memmove.
> + } else *locp = 0;
> + } else {
> + break;
> + }
> + }
Aside from the above, I'm concerned what happens when there are an
unexpected mix of _'s and @'s in places you don't expect them to be.
I've worked out an (untested) alternative approach which I think is
cleaner, where the locale name is split up into components before the
loop (in-place, with no locbuf) and the loop controls which parts get
used. This way the logic for how the parts are broken up is static and
not dependent on what happened in previous loop iterations.
> if (!map) goto notrans;
> +
> p = calloc(sizeof *p + namelen + 1, 1);
namelen space is not needed here anymore since name[] was removed from
the struct.
> if (!p) {
> __munmap((void *)map, map_size);
> @@ -178,7 +195,9 @@
> }
> + p->cat = category;
> + p->binding = q;
> + p->lm = lm;
> p->map = map;
> p->map_size = map_size;
> - memcpy(p->name, name, namelen+1);
> do {
> old_cats = cats;
> p->next = old_cats;
> --- musl-1.1.16/src/locale/locale_map.c 2017-01-01 03:27:17.000000000 +0000
> +++ musl-1.1.16/src/locale/locale_map.c 2017-01-01 03:27:17.000000000 +0000
> @@ -46,7 +46,8 @@
> if (val[0]=='.' || val[n]) val = "C.UTF-8";
> int builtin = (val[0]=='C' && !val[1])
> || !strcmp(val, "C.UTF-8")
> - || !strcmp(val, "POSIX");
> + || !strcmp(val, "POSIX")
> + || strcmp(__strchrnul(val, '.'), ".UTF-8");
>
> if (builtin) {
> if (cat == LC_CTYPE && val[1]=='.')
As discussed this is a separate change, and I don't think it's right
as-is. The right change requires adding an error path to the function
(and its callers), which I'll look into next.
Attached is my draft of modifications to your patch. I haven't tested
it yet but it does compile ok.
Rich
View attachment "dcngettext.diff" of type "text/plain" (3575 bytes)
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.
