| Message ID | 52938D8E.7020806@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25/11/13 12:49, Susant Sahani wrote: > Hi Steved, > I am sorry not for giving proper description . I have addressed your comments attached the patches . > Thanks, All three committed... Next time please send the patches in separate emails and in-line the patches instead of attaching them... Thanks! steved. > Susant > > > On 11/22/2013 09:58 PM, Steve Dickson wrote: >> Hello, >> >> Would it be possible to get a little better description as >> to what this patch does and why its needed... >> "data race in bindresvport_sa" have very little >> meaning, at least to me... >> >> More comments below... >> >> On 20/11/13 11:49, Susant Sahani wrote: >>> Signed-off-by: Susant Sahani <ssahani@redhat.com> >>> --- >>> src/bindresvport.c | 16 +++++++++++++--- >>> 1 file changed, 13 insertions(+), 3 deletions(-) >>> >>> diff --git a/src/bindresvport.c b/src/bindresvport.c >>> index 6ce3e81..d26d754 100644 >>> --- a/src/bindresvport.c >>> +++ b/src/bindresvport.c >>> @@ -46,6 +46,7 @@ >>> #include <rpc/rpc.h> >>> #include <string.h> >>> +#include <reentrant.h> >>> /* >>> * Bind a socket to a privileged IP port >>> @@ -79,17 +80,23 @@ bindresvport_sa(sd, sa) >>> u_int16_t *portp; >>> static u_int16_t port; >>> static short startport = STARTPORT; >>> + static pthread_mutex_t port_lock = PTHREAD_MUTEX_INITIALIZER; >> How come you define this mutex statically instead in src/mt_misc.c >> like the rest of the mutexes? >> >> Would you mind moving this (and the other two in the patches) >> to src/mt_misc.c and added a commit talking about what they >> are protecting >> >> tia! >> >> steved. >> >>> socklen_t salen; >>> - int nports = ENDPORT - startport + 1; >>> + int nports; >>> int endport = ENDPORT; >>> int i; >>> + mutex_lock(&port_lock); >>> + nports = ENDPORT - startport + 1; >>> + >>> if (sa == NULL) { >>> salen = sizeof(myaddr); >>> sa = (struct sockaddr *)&myaddr; >>> - if (getsockname(sd, (struct sockaddr *)&myaddr, &salen) == -1) >>> - return -1; /* errno is correctly set */ >>> + if (getsockname(sd, (struct sockaddr *)&myaddr, &salen) == -1) { >>> + mutex_unlock(&port_lock); >>> + return -1; /* errno is correctly set */ >>> + } >>> af = myaddr.ss_family; >>> } else >>> @@ -112,6 +119,7 @@ bindresvport_sa(sd, sa) >>> #endif >>> default: >>> errno = EPFNOSUPPORT; >>> + mutex_unlock(&port_lock); >>> return (-1); >>> } >>> sa->sa_family = af; >>> @@ -137,6 +145,8 @@ bindresvport_sa(sd, sa) >>> port = LOWPORT + port % (STARTPORT - LOWPORT); >>> goto again; >>> } >>> + mutex_unlock(&port_lock); >>> + >>> return (res); >>> } >>> #else >>> > > > > ------------------------------------------------------------------------------ > Shape the Mobile Experience: Free Subscription > Software experts and developers: Be at the forefront of tech innovation. > Intel(R) Software Adrenaline delivers strategic insight and game-changing > conversations that shape the rapidly evolving mobile landscape. Sign up now. > http://pubads.g.doubleclick.net/gampad/clk?id=63431311&iu=/4140/ostg.clktrk > > > > _______________________________________________ > Libtirpc-devel mailing list > Libtirpc-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/libtirpc-devel > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From f850621962cb57c7bebfc93bd28db1f26be213aa Mon Sep 17 00:00:00 2001 From: Susant Sahani <ssahani@redhat.com> Date: Sat, 23 Nov 2013 13:12:59 +0530 Subject: [PATCH 3/3] Race in Race in clnt_vc_create The function clnt_create is *not* thread safe. Race conditions in the function clnt_vc_create that accesses static data disrupt, which is *not* protected by any mutex. When more than one thread access it it has become a nonlocal side effect . This race conditions can lead to undesired behaviour . By introducing the mutex disrupt_lock the function clnt_vc_create is serialized Signed-off-by: Susant Sahani <ssahani@redhat.com> --- src/clnt_vc.c | 5 +++++ src/mt_misc.c | 3 +++ 2 files changed, 8 insertions(+) diff --git a/src/clnt_vc.c b/src/clnt_vc.c index 2eab9e4..cbbfc58 100644 --- a/src/clnt_vc.c +++ b/src/clnt_vc.c @@ -173,14 +173,17 @@ clnt_vc_create(fd, raddr, prog, vers, sendsz, recvsz) struct timeval now; struct rpc_msg call_msg; static u_int32_t disrupt; + extern pthread_mutex_t disrupt_lock; sigset_t mask; sigset_t newmask; struct sockaddr_storage ss; socklen_t slen; struct __rpc_sockinfo si; + mutex_lock(&disrupt_lock); if (disrupt == 0) disrupt = (u_int32_t)(long)raddr; + mutex_unlock(&disrupt_lock); cl = (CLIENT *)mem_alloc(sizeof (*cl)); ct = (struct ct_data *)mem_alloc(sizeof (*ct)); @@ -270,7 +273,9 @@ clnt_vc_create(fd, raddr, prog, vers, sendsz, recvsz) * Initialize call message */ (void)gettimeofday(&now, NULL); + mutex_lock(&disrupt_lock); call_msg.rm_xid = ((u_int32_t)++disrupt) ^ __RPC_GETXID(&now); + mutex_unlock(&disrupt_lock); call_msg.rm_direction = CALL; call_msg.rm_call.cb_rpcvers = RPC_MSG_VERSION; call_msg.rm_call.cb_prog = (u_int32_t)prog; diff --git a/src/mt_misc.c b/src/mt_misc.c index ddbb0a5..d459dec 100644 --- a/src/mt_misc.c +++ b/src/mt_misc.c @@ -97,6 +97,9 @@ pthread_mutex_t nc_db_lock = PTHREAD_MUTEX_INITIALIZER; /* protects static port and startport (bindresvport.c) */ pthread_mutex_t port_lock = PTHREAD_MUTEX_INITIALIZER; +/* protects static disrupt (clnt_vc.c) */ +pthread_mutex_t disrupt_lock = PTHREAD_MUTEX_INITIALIZER; + #undef rpc_createerr struct rpc_createerr rpc_createerr; -- 1.8.4.2
Hi Steved, I am sorry not for giving proper description . I have addressed your comments attached the patches . Thanks, Susant On 11/22/2013 09:58 PM, Steve Dickson wrote: > Hello, > > Would it be possible to get a little better description as > to what this patch does and why its needed... > "data race in bindresvport_sa" have very little > meaning, at least to me... > > More comments below... > > On 20/11/13 11:49, Susant Sahani wrote: >> Signed-off-by: Susant Sahani <ssahani@redhat.com> >> --- >> src/bindresvport.c | 16 +++++++++++++--- >> 1 file changed, 13 insertions(+), 3 deletions(-) >> >> diff --git a/src/bindresvport.c b/src/bindresvport.c >> index 6ce3e81..d26d754 100644 >> --- a/src/bindresvport.c >> +++ b/src/bindresvport.c >> @@ -46,6 +46,7 @@ >> #include <rpc/rpc.h> >> >> #include <string.h> >> +#include <reentrant.h> >> >> /* >> * Bind a socket to a privileged IP port >> @@ -79,17 +80,23 @@ bindresvport_sa(sd, sa) >> u_int16_t *portp; >> static u_int16_t port; >> static short startport = STARTPORT; >> + static pthread_mutex_t port_lock = PTHREAD_MUTEX_INITIALIZER; > How come you define this mutex statically instead in src/mt_misc.c > like the rest of the mutexes? > > Would you mind moving this (and the other two in the patches) > to src/mt_misc.c and added a commit talking about what they > are protecting > > tia! > > steved. > >> socklen_t salen; >> - int nports = ENDPORT - startport + 1; >> + int nports; >> int endport = ENDPORT; >> int i; >> >> + mutex_lock(&port_lock); >> + nports = ENDPORT - startport + 1; >> + >> if (sa == NULL) { >> salen = sizeof(myaddr); >> sa = (struct sockaddr *)&myaddr; >> >> - if (getsockname(sd, (struct sockaddr *)&myaddr, &salen) == -1) >> - return -1; /* errno is correctly set */ >> + if (getsockname(sd, (struct sockaddr *)&myaddr, &salen) == -1) { >> + mutex_unlock(&port_lock); >> + return -1; /* errno is correctly set */ >> + } >> >> af = myaddr.ss_family; >> } else >> @@ -112,6 +119,7 @@ bindresvport_sa(sd, sa) >> #endif >> default: >> errno = EPFNOSUPPORT; >> + mutex_unlock(&port_lock); >> return (-1); >> } >> sa->sa_family = af; >> @@ -137,6 +145,8 @@ bindresvport_sa(sd, sa) >> port = LOWPORT + port % (STARTPORT - LOWPORT); >> goto again; >> } >> + mutex_unlock(&port_lock); >> + >> return (res); >> } >> #else >>