Date: Thu, 19 Mar 2020 06:33:06 +0000 (UTC) From: Xin LI <delphij@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r359118 - head/lib/libc/nls Message-ID: <202003190633.02J6X6lC055665@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
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@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); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202003190633.02J6X6lC055665>