From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/11149 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: Re: a bug in bindtextdomain() and strip '.UTF-8' Date: Fri, 17 Mar 2017 15:27:49 -0400 Message-ID: <20170317192749.GL1693@brightrain.aerifal.cx> References: <20170208143147.GY1533@brightrain.aerifal.cx> <20170211023610.GA1520@brightrain.aerifal.cx> <20170212023422.GE1520@brightrain.aerifal.cx> <20170213132816.GG1520@brightrain.aerifal.cx> <20170213171236.GI1520@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="LwW0XdcUbUexiWVK" X-Trace: blaine.gmane.org 1489778903 24525 195.159.176.226 (17 Mar 2017 19:28:23 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 17 Mar 2017 19:28:23 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-11164-gllmg-musl=m.gmane.org@lists.openwall.com Fri Mar 17 20:28:18 2017 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1coxXt-0005oM-6f for gllmg-musl@m.gmane.org; Fri, 17 Mar 2017 20:28:17 +0100 Original-Received: (qmail 32099 invoked by uid 550); 17 Mar 2017 19:28:02 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 32053 invoked from network); 17 Mar 2017 19:28:01 -0000 Content-Disposition: inline In-Reply-To: Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:11149 Archived-At: --LwW0XdcUbUexiWVK Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 --LwW0XdcUbUexiWVK Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="dcngettext.diff" diff --git a/src/locale/dcngettext.c b/src/locale/dcngettext.c index b68e24b..abaa414 100644 --- a/src/locale/dcngettext.c +++ b/src/locale/dcngettext.c @@ -100,7 +100,9 @@ struct msgcat { size_t map_size; void *volatile plural_rule; volatile int nplurals; - char name[]; + struct binding *binding; + const struct __locale_map *lm; + int cat; }; static char *dummy_gettextdomain() @@ -120,8 +122,8 @@ char *dcngettext(const char *domainname, const char *msgid1, const char *msgid2, 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,55 +132,76 @@ char *dcngettext(const char *domainname, const char *msgid1, const char *msgid2, 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, *modname, *locp; + size_t dirlen, loclen, catlen, modlen, alt_modlen; 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]; + + /* Logically split @mod suffix from locale name. */ + modname = memchr(locname, '@', loclen); + if (!modname) modname = locname + loclen; + alt_modlen = modlen = loclen - (modname-locname); + loclen = modname-locname; + + /* Drop .charset identifier; it is not used. */ + const char *csp = memchr(locname, '.', loclen); + if (csp) loclen = csp-locname; + + char name[dirlen+1 + loclen+1 + catlen+1 + domlen+3 + 1]; + const void *map; + + for (;;) { + snprintf(name, sizeof name, "%s/%.*s%.*s/%s/%s.mo\0", + dirname, (int)loclen, locname, + (int)alt_modlen, modname, catname, domainname); + if (map = __map_file(name, &map_size)) break; + + /* Try dropping @mod, _YY, then both. */ + if (alt_modlen) { + alt_modlen = 0; + } else if ((locp = memchr(locname, '_', loclen))) { + loclen = locp-locname; + alt_modlen = modlen; + } else { + break; + } + } if (!map) goto notrans; - p = calloc(sizeof *p + namelen + 1, 1); + + p = calloc(sizeof *p, 1); if (!p) { __munmap((void *)map, map_size); goto notrans; } + 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; --LwW0XdcUbUexiWVK--