Re: Re: a bug in bindtextdomain() and strip '.UTF-8'

[Rich Felker <dalias@libc.org>]

[-- Attachment #1: Type: text/plain, Size: 5428 bytes --]

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

[-- Attachment #2: dcngettext.diff --]
[-- Type: text/plain, Size: 3574 bytes --]

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;