From b8662e8079b986630575aa088c402643a3bda628 Mon Sep 17 00:00:00 2001 From: "Signed-off-by: Susant Sahani" Date: Mon, 25 Nov 2013 13:48:32 -0500 Subject: [PATCH] Race conditions in getnetconfig The clnt_* functions are *not* thread safe. Race conditions are caused by the functions setnetconfig , getnetconfig, endnetconfig and getnetconfigent that accesses global static data nc_file and ni which are defined in the file getnetconfig are *not* protected by any mutex. When more than one thread access them the variables become a nonlocal side effect . These race conditions causing process to give undesired behavior and leading to crash on file operations mostly on fclose. By introducing the mutex nc_db_lock the netconfig database is synchronized and prevented from crash. Signed-off-by: Susant Sahani Signed-off-by: Steve Dickson --- src/getnetconfig.c | 57 +++++++++++++++++++++++++++++++++++----------- src/mt_misc.c | 3 +++ 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/src/getnetconfig.c b/src/getnetconfig.c index 2460a6e..78de0f6 100644 --- a/src/getnetconfig.c +++ b/src/getnetconfig.c @@ -120,6 +120,7 @@ static struct netconfig *dup_ncp(struct netconfig *); static FILE *nc_file; /* for netconfig db */ static struct netconfig_info ni = { 0, 0, NULL, NULL}; +extern pthread_mutex_t nc_db_lock; #define MAXNETCONFIGLINE 1000 @@ -192,14 +193,17 @@ setnetconfig() * For multiple calls, i.e. nc_file is not NULL, we just return the * handle without reopening the netconfig db. */ + mutex_lock(&nc_db_lock); ni.ref++; if ((nc_file != NULL) || (nc_file = fopen(NETCONFIG, "r")) != NULL) { nc_vars->valid = NC_VALID; nc_vars->flag = 0; nc_vars->nc_configs = ni.head; + mutex_unlock(&nc_db_lock); return ((void *)nc_vars); } ni.ref--; + mutex_unlock(&nc_db_lock); nc_error = NC_NONETCONFIG; free(nc_vars); return (NULL); @@ -222,12 +226,15 @@ void *handlep; char *stringp; /* tmp string pointer */ struct netconfig_list *list; struct netconfig *np; + struct netconfig *result; /* * Verify that handle is valid */ + mutex_lock(&nc_db_lock); if (ncp == NULL || nc_file == NULL) { nc_error = NC_NOTINIT; + mutex_unlock(&nc_db_lock); return (NULL); } @@ -244,11 +251,14 @@ void *handlep; if (ncp->flag == 0) { /* first time */ ncp->flag = 1; ncp->nc_configs = ni.head; - if (ncp->nc_configs != NULL) /* entry already exist */ + if (ncp->nc_configs != NULL) /* entry already exist */ { + mutex_unlock(&nc_db_lock); return(ncp->nc_configs->ncp); + } } else if (ncp->nc_configs != NULL && ncp->nc_configs->next != NULL) { ncp->nc_configs = ncp->nc_configs->next; + mutex_unlock(&nc_db_lock); return(ncp->nc_configs->ncp); } @@ -256,16 +266,22 @@ void *handlep; * If we cannot find the entry in the list and is end of file, * we give up. */ - if (ni.eof == 1) return(NULL); + if (ni.eof == 1) { + mutex_unlock(&nc_db_lock); + return(NULL); + } break; default: nc_error = NC_NOTINIT; + mutex_unlock(&nc_db_lock); return (NULL); } stringp = (char *) malloc(MAXNETCONFIGLINE); - if (stringp == NULL) - return (NULL); + if (stringp == NULL) { + mutex_unlock(&nc_db_lock); + return (NULL); + } #ifdef MEM_CHK if (malloc_verify() == 0) { @@ -281,6 +297,7 @@ void *handlep; if (fgets(stringp, MAXNETCONFIGLINE, nc_file) == NULL) { free(stringp); ni.eof = 1; + mutex_unlock(&nc_db_lock); return (NULL); } } while (*stringp == '#'); @@ -288,12 +305,14 @@ void *handlep; list = (struct netconfig_list *) malloc(sizeof (struct netconfig_list)); if (list == NULL) { free(stringp); + mutex_unlock(&nc_db_lock); return(NULL); } np = (struct netconfig *) malloc(sizeof (struct netconfig)); if (np == NULL) { free(stringp); - free(list); + free(list); + mutex_unlock(&nc_db_lock); return(NULL); } list->ncp = np; @@ -304,6 +323,7 @@ void *handlep; free(stringp); free(np); free(list); + mutex_unlock(&nc_db_lock); return (NULL); } else { @@ -321,7 +341,9 @@ void *handlep; ni.tail = ni.tail->next; } ncp->nc_configs = ni.tail; - return(ni.tail->ncp); + result = ni.tail->ncp; + mutex_unlock(&nc_db_lock); + return result; } } @@ -355,8 +377,10 @@ void *handlep; nc_handlep->valid = NC_INVALID; nc_handlep->flag = 0; nc_handlep->nc_configs = NULL; + mutex_lock(&nc_db_lock); if (--ni.ref > 0) { - free(nc_handlep); + mutex_unlock(&nc_db_lock); + free(nc_handlep); return(0); } @@ -377,9 +401,11 @@ void *handlep; q = p; } free(nc_handlep); - - fclose(nc_file); + if(nc_file != NULL) { + fclose(nc_file); + } nc_file = NULL; + mutex_unlock(&nc_db_lock); return (0); } @@ -427,16 +453,21 @@ getnetconfigent(netid) * If all the netconfig db has been read and placed into the list and * there is no match for the netid, return NULL. */ + mutex_lock(&nc_db_lock); if (ni.head != NULL) { for (list = ni.head; list; list = list->next) { if (strcmp(list->ncp->nc_netid, netid) == 0) { - return(dup_ncp(list->ncp)); + ncp = dup_ncp(list->ncp); + mutex_unlock(&nc_db_lock); + return ncp; } } - if (ni.eof == 1) /* that's all the entries */ - return(NULL); + if (ni.eof == 1) { /* that's all the entries */ + mutex_unlock(&nc_db_lock); + return(NULL); + } } - + mutex_unlock(&nc_db_lock); if ((file = fopen(NETCONFIG, "r")) == NULL) { nc_error = NC_NONETCONFIG; diff --git a/src/mt_misc.c b/src/mt_misc.c index fe12c31..b24c130 100644 --- a/src/mt_misc.c +++ b/src/mt_misc.c @@ -91,6 +91,9 @@ pthread_mutex_t xprtlist_lock = PTHREAD_MUTEX_INITIALIZER; /* serializes calls to public key routines */ pthread_mutex_t serialize_pkey = PTHREAD_MUTEX_INITIALIZER; +/* protects global variables ni and nc_file (getnetconfig.c) */ +pthread_mutex_t nc_db_lock = PTHREAD_MUTEX_INITIALIZER; + #undef rpc_createerr struct rpc_createerr rpc_createerr; -- 2.40.0