From owner-svn-src-head@freebsd.org Thu Mar 19 06:33:07 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 71F9227CA6F; Thu, 19 Mar 2020 06:33:07 +0000 (UTC) (envelope-from delphij@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 48jcXy6y6tz3QZb; Thu, 19 Mar 2020 06:33:06 +0000 (UTC) (envelope-from delphij@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id D1F336D03; Thu, 19 Mar 2020 06:33:06 +0000 (UTC) (envelope-from delphij@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id 02J6X6Gn055666; Thu, 19 Mar 2020 06:33:06 GMT (envelope-from delphij@FreeBSD.org) Received: (from delphij@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id 02J6X6lC055665; Thu, 19 Mar 2020 06:33:06 GMT (envelope-from delphij@FreeBSD.org) Message-Id: <202003190633.02J6X6lC055665@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: delphij set sender to delphij@FreeBSD.org using -f From: Xin LI Date: Thu, 19 Mar 2020 06:33:06 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r359118 - head/lib/libc/nls X-SVN-Group: head X-SVN-Commit-Author: delphij X-SVN-Commit-Paths: head/lib/libc/nls X-SVN-Commit-Revision: 359118 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Mar 2020 06:33:07 -0000 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 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 #include /* for ntohl() */ +#include #include #include @@ -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); }