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...@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);
 }
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Reply via email to