svn commit: r359118 - head/lib/libc/nls

[Xin LI]

Author: delphij
Date: Thu Mar 19 06:33:06 2020
New Revision: 359118
URL: https://svnweb.freebsd.org/changeset/base/359118

Log:
  Fix race condition in catopen(3).
  
  The current code uses a rwlock to protect the cached list, which
  in turn holds a list of catentry objects, and increments reference
  count while holding only read lock.
  
  Fix this by converting the reference counter to use atomic operations.
  
  While I'm there, also perform some clean ups around memory operations.
  
  PR:		202636
  Reported by:	Henry Hu <henry.hu.sh at gmail.com>
  Reviewed by:	markj
  MFC after:	2 weeks
  Differential Revision:	https://reviews.freebsd.org/D24095

Modified:
  head/lib/libc/nls/msgcat.c

Modified: head/lib/libc/nls/msgcat.c
==============================================================================
--- head/lib/libc/nls/msgcat.c	Thu Mar 19 03:37:02 2020	(r359117)
+++ head/lib/libc/nls/msgcat.c	Thu Mar 19 06:33:06 2020	(r359118)
@@ -43,6 +43,7 @@ __FBSDID("$FreeBSD$");
 #include <sys/queue.h>
 
 #include <arpa/inet.h>		/* for ntohl() */
+#include <machine/atomic.h>
 
 #include <errno.h>
 #include <fcntl.h>
@@ -76,19 +77,25 @@ __FBSDID("$FreeBSD$");
 
 #define	NLERR		((nl_catd) -1)
 #define NLRETERR(errc)  { errno = errc; return (NLERR); }
-#define SAVEFAIL(n, l, e)	{ WLOCK(NLERR);					\
-				  np = malloc(sizeof(struct catentry));		\
+#define SAVEFAIL(n, l, e)	{ np = calloc(1, sizeof(struct catentry));	\
 				  if (np != NULL) {				\
 				  	np->name = strdup(n);			\
-					np->path = NULL;			\
 					np->catd = NLERR;			\
-					np->refcount = 0;			\
 					np->lang = (l == NULL) ? NULL :		\
 					    strdup(l);				\
 					np->caterrno = e;			\
-				  	SLIST_INSERT_HEAD(&cache, np, list);	\
+					if (np->name == NULL ||			\
+					    (l != NULL && np->lang == NULL)) {	\
+						free(np->name);			\
+						free(np->lang);			\
+						free(np);			\
+					} else {				\
+						WLOCK(NLERR);			\
+						SLIST_INSERT_HEAD(&cache, np,	\
+						    list);			\
+						UNLOCK;				\
+					}					\
 				  }						\
-				  UNLOCK;					\
 				  errno = e;					\
 				}
 
@@ -152,7 +159,7 @@ catopen(const char *name, int type)
 				NLRETERR(np->caterrno);
 			} else {
 				/* Found cached successful entry */
-				np->refcount++;
+				atomic_add_int(&np->refcount, 1);
 				UNLOCK;
 				return (np->catd);
 			}
@@ -355,8 +362,7 @@ catclose(nl_catd catd)
 	WLOCK(-1);
 	SLIST_FOREACH(np, &cache, list) {
 		if (catd == np->catd) {
-			np->refcount--;
-			if (np->refcount == 0)
+			if (atomic_fetchadd_int(&np->refcount, -1) == 1)
 				catfree(np);
 			break;
 		}
@@ -376,6 +382,7 @@ load_msgcat(const char *path, const char *name, const 
 	nl_catd	catd;
 	struct catentry *np;
 	void *data;
+	char *copy_path, *copy_name, *copy_lang;
 	int fd;
 
 	/* path/name will never be NULL here */
@@ -387,7 +394,7 @@ load_msgcat(const char *path, const char *name, const 
 	RLOCK(NLERR);
 	SLIST_FOREACH(np, &cache, list) {
 		if ((np->path != NULL) && (strcmp(np->path, path) == 0)) {
-			np->refcount++;
+			atomic_add_int(&np->refcount, 1);
 			UNLOCK;
 			return (np->catd);
 		}
@@ -432,7 +439,20 @@ load_msgcat(const char *path, const char *name, const 
 		NLRETERR(EFTYPE);
 	}
 
-	if ((catd = malloc(sizeof (*catd))) == NULL) {
+	copy_name = strdup(name);
+	copy_path = strdup(path);
+	copy_lang = (lang == NULL) ? NULL : strdup(lang);
+	catd = malloc(sizeof (*catd));
+	np = calloc(1, sizeof(struct catentry));
+
+	if (copy_name == NULL || copy_path == NULL ||
+	    (lang != NULL && copy_lang == NULL) ||
+	    catd == NULL || np == NULL) {
+		free(copy_name);
+		free(copy_path);
+		free(copy_lang);
+		free(catd);
+		free(np);
 		munmap(data, (size_t)st.st_size);
 		SAVEFAIL(name, lang, ENOMEM);
 		NLRETERR(ENOMEM);
@@ -442,16 +462,13 @@ load_msgcat(const char *path, const char *name, const 
 	catd->__size = (int)st.st_size;
 
 	/* Caching opened catalog */
+	np->name = copy_name;
+	np->path = copy_path;
+	np->catd = catd;
+	np->lang = copy_lang;
+	atomic_store_int(&np->refcount, 1);
 	WLOCK(NLERR);
-	if ((np = malloc(sizeof(struct catentry))) != NULL) {
-		np->name = strdup(name);
-		np->path = strdup(path);
-		np->catd = catd;
-		np->lang = (lang == NULL) ? NULL : strdup(lang);
-		np->refcount = 1;
-		np->caterrno = 0;
-		SLIST_INSERT_HEAD(&cache, np, list);
-	}
+	SLIST_INSERT_HEAD(&cache, np, list);
 	UNLOCK;
 	return (catd);
 }