Re: [PATCH] libc: make I/O locking cancel-safe with futexes

[Rich Felker]

On Wed, Sep 12, 2012 at 04:55:29PM +0200, Carmelo AMOROSO wrote:
> From: Filippo ARCIDIACONO 
> 
> Current implementation of I/O locking macros using futexes are not
> cancel safe, as if a thread is cancelled while doing IO operations,
> it does not release the lock on the IO stream. So following
> attempts to acquire the IO stream's lock would dead-lock.
> In order to make it cancel-safe, it needs to rely upon the
> pthread_cleanup push & pop protocol, as it is already done for
> the pthread_mutex counterpart.

This is an unnecessarily heavy approach. Cancellation cannot be acted
upon unless a call to a cancellation point is made, so you do not need
to install cleanup handlers whenever the file is locked, only when
cancellable actions will be performed under lock, i.e. when the buffer
needs to be flushed or filled. Perhaps it matters less with NPTL's
exception-based cancellation, but I still think it's worth considering.

Rich
___
uClibc mailing list
uClibc@uclibc.org
http://lists.busybox.net/mailman/listinfo/uclibc

RE: [PATCH] libc: make I/O locking cancel-safe with futexes

[Filippo ARCIDIACONO]

Could someone take a look?
Further details could be found at
https://bugzilla.stlinux.com/show_bug.cgi?id=21083
there is also a test case spotting the issue.

> -Original Message-
> From: Carmelo AMOROSO [mailto:carmelo.amor...@st.com]
> Sent: Wednesday, September 12, 2012 4:55 PM
> To: uclibc@uclibc.org
> Cc: Filippo ARCIDIACONO; Carmelo Amoroso
> Subject: [PATCH] libc: make I/O locking cancel-safe with futexes
> 
> From: Filippo ARCIDIACONO 
> 
> Current implementation of I/O locking macros using futexes are not
> cancel safe, as if a thread is cancelled while doing IO operations,
> it does not release the lock on the IO stream. So following
> attempts to acquire the IO stream's lock would dead-lock.
> In order to make it cancel-safe, it needs to rely upon the
> pthread_cleanup push & pop protocol, as it is already done for
> the pthread_mutex counterpart.
> 
> Signed-off-by: Filippo Arcidiacono 
> Signed-off-by: Carmelo Amoroso 
> ---
>  libc/stdio/Makefile.in|3 ++
>  libc/stdio/io_lock_unlock.c   |   16 ++
>  libc/sysdeps/linux/common/bits/uClibc_mutex.h |   23 +---
> 
>  libpthread/nptl/sysdeps/pthread/bits/stdio-lock.h |2 +
>  4 files changed, 36 insertions(+), 8 deletions(-)
>  create mode 100644 libc/stdio/io_lock_unlock.c
> 
> diff --git a/libc/stdio/Makefile.in b/libc/stdio/Makefile.in
> index ff77bcb..4652036 100644
> --- a/libc/stdio/Makefile.in
> +++ b/libc/stdio/Makefile.in
> @@ -52,6 +52,9 @@ endif
> 
>  # pthread functions
>  CSRC += flockfile.c ftrylockfile.c funlockfile.c
> +ifeq ($(UCLIBC_HAS_STDIO_FUTEXES),y)
> +CSRC += io_lock_unlock.c
> +endif
> 
>  # Functions with unlocked versions
>  CUSRC := \
> diff --git a/libc/stdio/io_lock_unlock.c b/libc/stdio/io_lock_unlock.c
> new file mode 100644
> index 000..b414264
> --- /dev/null
> +++ b/libc/stdio/io_lock_unlock.c
> @@ -0,0 +1,16 @@
> +/*
> + * Copyright (C) 2012 STMicroelectronics, Ltd
> + *
> + * Author(s): Filippo Arcidiacono  
> + *
> + * Licensed under the LGPL v2.1, see the file COPYING.LIB in this tarball
> + */
> +
> +#include 
> +
> +/* Function wrapper needed for I/O locking macros. */
> +
> +void IO_lock_unlock (_IO_lock_t *lock)
> +{
> +  _IO_lock_unlock (*lock);
> +}
> diff --git a/libc/sysdeps/linux/common/bits/uClibc_mutex.h
> b/libc/sysdeps/linux/common/bits/uClibc_mutex.h
> index 94597e8..c2a1cc1 100644
> --- a/libc/sysdeps/linux/common/bits/uClibc_mutex.h
> +++ b/libc/sysdeps/linux/common/bits/uClibc_mutex.h
> @@ -12,6 +12,7 @@
> 
>  #ifdef __UCLIBC_HAS_THREADS__
> 
> +
>  #include 
>  #ifdef _LIBC
>  #include 
> @@ -79,15 +80,21 @@
>  #define __UCLIBC_IO_MUTEX_INIT(M)_IO_lock_t M =
> _IO_lock_initializer
>  #define __UCLIBC_IO_MUTEX_EXTERN(M)  extern _IO_lock_t M
> 
> -#define __UCLIBC_IO_MUTEX_CONDITIONAL_LOCK(M,C)  \
> - if (C) {
>   \
> - _IO_lock_lock(M);
>   \
> - }
> +#define __UCLIBC_IO_MUTEX_CONDITIONAL_LOCK(M,C)
>   \
> + do {
>   \
> + struct _pthread_cleanup_buffer
> __infunc_pthread_cleanup_buffer;  \
> + int __infunc_need_locking = (C);
>   \
> + if (__infunc_need_locking) {
>   \
> +
>   _pthread_cleanup_push_defer(&__infunc_pthread_cleanup_buffer
> , \
> + (void (*) (void
> *))IO_lock_unlock,\
> + &(M));
>   \
> + _IO_lock_lock((M));
>   \
> + }
>   \
> + ((void)0)
> +
> 
> -#define __UCLIBC_IO_MUTEX_CONDITIONAL_UNLOCK(M,C)\
> - if (C) {
>   \
> - _IO_lock_unlock(M);
>   \
> - }
> +#define __UCLIBC_IO_MUTEX_CONDITIONAL_UNLOCK(M,C)
>   \
> + __UCLIBC_MUTEX_CONDITIONAL_UNLOCK(M,C)
> 
>  #define __UCLIBC_IO_MUTEX_AUTO_LOCK(M,A,V)   \
>   __UCLIBC_IO_MUTEX_CONDITIONAL_LOCK(M,((A=(V))) ==
> 0)
> diff --git a/libpthread/nptl/sysdeps/pthread/bits/stdio-lock.h
> b/libpthread/nptl/sysdeps/pthread/bits/stdio-lock.h
> index 3437e61..c742644 100644
> --- a/libpthread/nptl/sysdeps/pthread/bits/stdio-lock.h
> +++ b/libpthread/nptl/sysdeps/pthread/bits/stdio-lock.h
> @@ -29,6 +29,8 @@
> 
>  typedef struct { int lock; int cnt; void *owner; } _IO_lock_t;
> 
> +void IO_lock_unlock (_IO_lock_t *lock);
> +
>  #define _IO_lock_initializer { LLL_LOCK_INITIALIZER, 0, NULL }
> 
>  #define _IO_lock_init(_name) \
> --
> 1.7.4.4


___
uClibc mailing list
uClibc@uclibc.org
http://lists.busybox.net/mailman/listinfo/uclibc

[PATCH] libc: make I/O locking cancel-safe with futexes

[Carmelo AMOROSO]

From: Filippo ARCIDIACONO 

Current implementation of I/O locking macros using futexes are not
cancel safe, as if a thread is cancelled while doing IO operations,
it does not release the lock on the IO stream. So following
attempts to acquire the IO stream's lock would dead-lock.
In order to make it cancel-safe, it needs to rely upon the
pthread_cleanup push & pop protocol, as it is already done for
the pthread_mutex counterpart.

Signed-off-by: Filippo Arcidiacono 
Signed-off-by: Carmelo Amoroso 
---
 libc/stdio/Makefile.in|3 ++
 libc/stdio/io_lock_unlock.c   |   16 ++
 libc/sysdeps/linux/common/bits/uClibc_mutex.h |   23 +---
 libpthread/nptl/sysdeps/pthread/bits/stdio-lock.h |2 +
 4 files changed, 36 insertions(+), 8 deletions(-)
 create mode 100644 libc/stdio/io_lock_unlock.c

diff --git a/libc/stdio/Makefile.in b/libc/stdio/Makefile.in
index ff77bcb..4652036 100644
--- a/libc/stdio/Makefile.in
+++ b/libc/stdio/Makefile.in
@@ -52,6 +52,9 @@ endif
 
 # pthread functions
 CSRC += flockfile.c ftrylockfile.c funlockfile.c
+ifeq ($(UCLIBC_HAS_STDIO_FUTEXES),y)
+CSRC += io_lock_unlock.c
+endif
 
 # Functions with unlocked versions
 CUSRC := \
diff --git a/libc/stdio/io_lock_unlock.c b/libc/stdio/io_lock_unlock.c
new file mode 100644
index 000..b414264
--- /dev/null
+++ b/libc/stdio/io_lock_unlock.c
@@ -0,0 +1,16 @@
+/*
+ * Copyright (C) 2012 STMicroelectronics, Ltd
+ *
+ * Author(s): Filippo Arcidiacono  
+ *
+ * Licensed under the LGPL v2.1, see the file COPYING.LIB in this tarball
+ */
+
+#include 
+
+/* Function wrapper needed for I/O locking macros. */
+
+void IO_lock_unlock (_IO_lock_t *lock)
+{
+  _IO_lock_unlock (*lock);
+}
diff --git a/libc/sysdeps/linux/common/bits/uClibc_mutex.h 
b/libc/sysdeps/linux/common/bits/uClibc_mutex.h
index 94597e8..c2a1cc1 100644
--- a/libc/sysdeps/linux/common/bits/uClibc_mutex.h
+++ b/libc/sysdeps/linux/common/bits/uClibc_mutex.h
@@ -12,6 +12,7 @@
 
 #ifdef __UCLIBC_HAS_THREADS__
 
+
 #include 
 #ifdef _LIBC
 #include 
@@ -79,15 +80,21 @@
 #define __UCLIBC_IO_MUTEX_INIT(M)  _IO_lock_t M = _IO_lock_initializer
 #define __UCLIBC_IO_MUTEX_EXTERN(M)extern _IO_lock_t M
 
-#define __UCLIBC_IO_MUTEX_CONDITIONAL_LOCK(M,C)\
-   if (C) {
\
-   _IO_lock_lock(M);   
\
-   }
+#define __UCLIBC_IO_MUTEX_CONDITIONAL_LOCK(M,C)
\
+   do {
\
+   struct _pthread_cleanup_buffer __infunc_pthread_cleanup_buffer; 
\
+   int __infunc_need_locking = (C);
\
+   if (__infunc_need_locking) {
\
+   
_pthread_cleanup_push_defer(&__infunc_pthread_cleanup_buffer,   \
+   (void (*) (void 
*))IO_lock_unlock,  \
+   &(M));  
\
+   _IO_lock_lock((M)); 
\
+   }   
\
+   ((void)0)
+
 
-#define __UCLIBC_IO_MUTEX_CONDITIONAL_UNLOCK(M,C)  \
-   if (C) {
\
-   _IO_lock_unlock(M); 
\
-   }
+#define __UCLIBC_IO_MUTEX_CONDITIONAL_UNLOCK(M,C)  \
+   __UCLIBC_MUTEX_CONDITIONAL_UNLOCK(M,C)
 
 #define __UCLIBC_IO_MUTEX_AUTO_LOCK(M,A,V) \
__UCLIBC_IO_MUTEX_CONDITIONAL_LOCK(M,((A=(V))) == 0)
diff --git a/libpthread/nptl/sysdeps/pthread/bits/stdio-lock.h 
b/libpthread/nptl/sysdeps/pthread/bits/stdio-lock.h
index 3437e61..c742644 100644
--- a/libpthread/nptl/sysdeps/pthread/bits/stdio-lock.h
+++ b/libpthread/nptl/sysdeps/pthread/bits/stdio-lock.h
@@ -29,6 +29,8 @@
 
 typedef struct { int lock; int cnt; void *owner; } _IO_lock_t;
 
+void IO_lock_unlock (_IO_lock_t *lock);
+
 #define _IO_lock_initializer { LLL_LOCK_INITIALIZER, 0, NULL }
 
 #define _IO_lock_init(_name) \
-- 
1.7.4.4

___
uClibc mailing list
uClibc@uclibc.org
http://lists.busybox.net/mailman/listinfo/uclibc