d/freebsd-skq
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix race condition in catopen(3).

Browse Source 
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@gmail.com>
Reviewed by:	markj
MFC after:	2 weeks
Differential Revision:	https://reviews.freebsd.org/D24095
This commit is contained in:
Xin LI 2020-03-19 06:33:06 +00:00
parent d3c6ba3214
commit 4188ba1a3b
1 changed files with 37 additions and 20 deletions

57
lib/libc/nls/msgcat.c

@ -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 char *lang)
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 char *lang)
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 char *lang)
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 char *lang)
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);
}