[PATCH] Make bindresvport() function to multithread-safe

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Make bindresvport() function to multithread-safe

Peng Haitao

bindresvport() uses two static variables port and startport which are not
protected. It is not safe when in multithread circumstance.

bindresvport() select a port number from the range 512 to 1023, when in
multithread circumstance, the port may be 1024. So the static variables will be
protected.

Signed-off-by: Peng Haitao <[hidden email]>
---
 ChangeLog           |    4 ++++
 sunrpc/bindrsvprt.c |   10 ++++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 58d02d6..42bfe3a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2012-02-16  Peng Haitao  <[hidden email]>
+
+ * sunrpc/bindrsvprt.c: Add lock to protect static variable.
+
 2012-02-16  Richard Henderson  <[hidden email]>
 
  * sysdeps/s390/s390-32/crti.S, sysdeps/s390/s390-32/crtn.S: New files.
diff --git a/sunrpc/bindrsvprt.c b/sunrpc/bindrsvprt.c
index d493c9f..f2fdefb 100644
--- a/sunrpc/bindrsvprt.c
+++ b/sunrpc/bindrsvprt.c
@@ -35,6 +35,12 @@
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
+#include <bits/libc-lock.h>
+
+/*
+ * Locks the static variables in this file.
+ */
+__libc_lock_define_initialized (static, lock);
 
 /*
  * Bind a socket to a privileged IP port
@@ -64,6 +70,8 @@ bindresvport (int sd, struct sockaddr_in *sin)
       return -1;
     }
 
+  __libc_lock_lock (lock);
+
   if (port == 0)
     {
       port = (__getpid () % NPORTS) + STARTPORT;
@@ -94,6 +102,8 @@ bindresvport (int sd, struct sockaddr_in *sin)
       goto again;
     }
 
+  __libc_lock_unlock (lock);
+
   return res;
 }
 libc_hidden_def (bindresvport)
--
1.7.1

--
Best Regards,
Peng

Reply | Threaded
Open this post in threaded view
|

Ping Re: [PATCH] Make bindresvport() function to multithread-safe

Peng Haitao

On 02/17/2012 02:00 PM, Peng Haitao wrote:
>
> bindresvport() uses two static variables port and startport which are not
> protected. It is not safe when in multithread circumstance.
>
> bindresvport() select a port number from the range 512 to 1023, when in
> multithread circumstance, the port may be 1024. So the static variables will be
> protected.
>

Ping.

--
Best Regards,
Peng

> Signed-off-by: Peng Haitao <[hidden email]>
> ---
>  ChangeLog           |    4 ++++
>  sunrpc/bindrsvprt.c |   10 ++++++++++
>  2 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 58d02d6..42bfe3a 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,7 @@
> +2012-02-16  Peng Haitao  <[hidden email]>
> +
> + * sunrpc/bindrsvprt.c: Add lock to protect static variable.
> +
>  2012-02-16  Richard Henderson  <[hidden email]>
>  
>   * sysdeps/s390/s390-32/crti.S, sysdeps/s390/s390-32/crtn.S: New files.
> diff --git a/sunrpc/bindrsvprt.c b/sunrpc/bindrsvprt.c
> index d493c9f..f2fdefb 100644
> --- a/sunrpc/bindrsvprt.c
> +++ b/sunrpc/bindrsvprt.c
> @@ -35,6 +35,12 @@
>  #include <sys/types.h>
>  #include <sys/socket.h>
>  #include <netinet/in.h>
> +#include <bits/libc-lock.h>
> +
> +/*
> + * Locks the static variables in this file.
> + */
> +__libc_lock_define_initialized (static, lock);
>  
>  /*
>   * Bind a socket to a privileged IP port
> @@ -64,6 +70,8 @@ bindresvport (int sd, struct sockaddr_in *sin)
>        return -1;
>      }
>  
> +  __libc_lock_lock (lock);
> +
>    if (port == 0)
>      {
>        port = (__getpid () % NPORTS) + STARTPORT;
> @@ -94,6 +102,8 @@ bindresvport (int sd, struct sockaddr_in *sin)
>        goto again;
>      }
>  
> +  __libc_lock_unlock (lock);
> +
>    return res;
>  }
>  libc_hidden_def (bindresvport)
>

--
Best Regards,
Peng
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Make bindresvport() function to multithread-safe

Mike Frysinger
In reply to this post by Peng Haitao
On Friday 17 February 2012 01:00:45 Peng Haitao wrote:
> @@ -64,6 +70,8 @@ bindresvport (int sd, struct sockaddr_in *sin)
>        return -1;
>      }
>
> +  __libc_lock_lock (lock);
> +
>    if (port == 0)
>      {
>        port = (__getpid () % NPORTS) + STARTPORT;

should the lock be acquired here ?  or should it be moved to right before the
"again" label ?  i think the latter ...
-mike

signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Ping Re: [PATCH] Make bindresvport() function to multithread-safe

Roland McGrath-4
In reply to this post by Peng Haitao
bindresvport has never been thread-safe before and AFAIK no other system
that has the function makes it thread-safe.  Like all functions, if it's
not specifically documented and being thread-safe, it isn't.  I don't see
the rationale for requiring bindresvport to be thread-safe now.
Reply | Threaded
Open this post in threaded view
|

Re: Ping Re: [PATCH] Make bindresvport() function to multithread-safe

Carlos O'Donell-4
On 9/17/2012 3:44 PM, Roland McGrath wrote:
> bindresvport has never been thread-safe before and AFAIK no other system
> that has the function makes it thread-safe.  Like all functions, if it's
> not specifically documented and being thread-safe, it isn't.  I don't see
> the rationale for requiring bindresvport to be thread-safe now.
>

I agree. If there is no known implementation that requires it to be thread-safe
then we should not either. It slows down applications that properly ensure
that only one thread uses bindresvport.

Cheers,
Carlos.
--
Carlos O'Donell
Mentor Graphics / CodeSourcery
[hidden email]
[hidden email]
+1 (613) 963 1026
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Make bindresvport() function to multithread-safe

Peng Haitao
In reply to this post by Mike Frysinger

On 09/18/2012 03:04 AM, Mike Frysinger wrote:

> On Friday 17 February 2012 01:00:45 Peng Haitao wrote:
>> @@ -64,6 +70,8 @@ bindresvport (int sd, struct sockaddr_in *sin)
>>        return -1;
>>      }
>>
>> +  __libc_lock_lock (lock);
>> +
>>    if (port == 0)
>>      {
>>        port = (__getpid () % NPORTS) + STARTPORT;
>
> should the lock be acquired here ?  or should it be moved to right before the
> "again" label ?  i think the latter ...
> -mike
>

Yeah, thanks.
I will send a new patch.

--
Best Regards,
Peng
Reply | Threaded
Open this post in threaded view
|

Re: Ping Re: [PATCH] Make bindresvport() function to multithread-safe

Peng Haitao
In reply to this post by Roland McGrath-4

On 09/18/2012 03:44 AM, Roland McGrath wrote:
> bindresvport has never been thread-safe before and AFAIK no other system
> that has the function makes it thread-safe.  Like all functions, if it's
> not specifically documented and being thread-safe, it isn't.  I don't see
> the rationale for requiring bindresvport to be thread-safe now.
>

Although I do not find any rationale for requiring bindresvport to be
thread-safe, bindresvport man-page says:
bindresvport() is used to bind a socket descriptor to a privileged
anonymous IP port, that is, a port number arbitrarily selected from the
range 512 to 1023.

That is, the valid value of port number is 512 to 1023,
but in multithread circumstance, the port number may be 1024.
The test program can refer to URL:
http://sourceware.org/bugzilla/show_bug.cgi?id=13763

So I think bindresvport() need to be fixed.

--
Best Regards,
Peng
Reply | Threaded
Open this post in threaded view
|

Re: Ping Re: [PATCH] Make bindresvport() function to multithread-safe

Andreas Schwab-2
Peng Haitao <[hidden email]> writes:

> That is, the valid value of port number is 512 to 1023,
> but in multithread circumstance, the port number may be 1024.
> The test program can refer to URL:
> http://sourceware.org/bugzilla/show_bug.cgi?id=13763
>
> So I think bindresvport() need to be fixed.

How can the use of a non-multi-thread-safe function in multiple threads
be a valid test case?

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: Ping Re: [PATCH] Make bindresvport() function to multithread-safe

Peng Haitao

On 10/09/2012 05:22 PM, Andreas Schwab wrote:

> Peng Haitao <[hidden email]> writes:
>
>> That is, the valid value of port number is 512 to 1023,
>> but in multithread circumstance, the port number may be 1024.
>> The test program can refer to URL:
>> http://sourceware.org/bugzilla/show_bug.cgi?id=13763
>>
>> So I think bindresvport() need to be fixed.
>
> How can the use of a non-multi-thread-safe function in multiple threads
> be a valid test case?
>

Can you tell me where I find restriction for bindresvport is not used
in multithread circumstance?

--
Best Regards,
Peng
Reply | Threaded
Open this post in threaded view
|

Re: Ping Re: [PATCH] Make bindresvport() function to multithread-safe

Andreas Schwab-2
Peng Haitao <[hidden email]> writes:

> Can you tell me where I find restriction for bindresvport is not used
> in multithread circumstance?

bindresvport is not specified by POSIX, so it cannot be assumed to be
thread-safe, unless there is a document stating that.

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: Ping Re: [PATCH] Make bindresvport() function to multithread-safe

Peng Haitao

On 10/10/2012 02:54 PM, Andreas Schwab wrote:
> Peng Haitao <[hidden email]> writes:
>
>> Can you tell me where I find restriction for bindresvport is not used
>> in multithread circumstance?
>
> bindresvport is not specified by POSIX, so it cannot be assumed to be
> thread-safe, unless there is a document stating that.
>

That is, if a function is not specified by POSIX, the function can not
be used in multithread circumstance.

Right?

Or this is a rationale.


--
Best Regards,
Peng
Reply | Threaded
Open this post in threaded view
|

Re: Ping Re: [PATCH] Make bindresvport() function to multithread-safe

Florian Weimer-5
In reply to this post by Andreas Schwab-2
On 10/10/2012 08:54 AM, Andreas Schwab wrote:
> Peng Haitao <[hidden email]> writes:
>
>> Can you tell me where I find restriction for bindresvport is not used
>> in multithread circumstance?
>
> bindresvport is not specified by POSIX, so it cannot be assumed to be
> thread-safe, unless there is a document stating that.

On the other hand, bindresvport doesn't take any argument and has no
notion of an object on which it works, so there is no obvious location
where to put the lock protecting it, and it is unlikely that two
different libraries which call bindresvport have a way to agree on the
lock to use.  So if it is acceptable at all to use bindresvport in
multi-threaded programs, it has to perform locking internally.  (But
this is purely a quality-of-implementation issue.)

--
Florian Weimer / Red Hat Product Security Team