diff mbox

Enable lightweight checks with _GLIBCXX_ASSERTIONS.

Message ID 20150926195209.GL12094@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely Sept. 26, 2015, 7:52 p.m. UTC 
On 14/09/15 11:57 +0200, Florian Weimer wrote:
>On 09/10/2015 06:57 PM, Martin Sebor wrote:
>
>>>> There is quite a bit of documentation of _FORTIFY_SOURCE that explains
>>>> its effect on user code.
>>>
>>> I think there are only random blog articles discussing aspects of it,
>>> most of them slightly incorrect or outdated.
>>
>> _FORTIFY_SOURCE is a GLIBC feature test macro. It's documented
>> in <features.h> and mentioned in some of its online manuals.
>> For example:
>>
>> http://man7.org/linux/man-pages/man7/feature_test_macros.7.html
>>
>> or here:
>>
>> http://manpages.ubuntu.com/manpages/hardy/man7/feature_test_macros.7.html
>
>Oh, so there is an out-dated man-page as well. :-/
>
>The fd_set checks added in glibc 2.15 are missing.  That caused some
>backslash because some folks were actually abusing FD_SET and related
>macros.  Nothing too severe, and in the end, we stood our ground.  I
>expect the libstdc++ changes to be similar.
>
>Again, my main argument is that the main users of _FORTIFY_SOURCE are
>distributions, and they would inject whatever preprocessor macro enables
>the new libstdc++ checks anyway, so saving them that work would be
>preferable IMHO.

Would changes like this be suitable for _FORTIFY_SOURCE?

Comments

Florian Weimer Sept. 26, 2015, 8:49 p.m. UTC | #1 
On 09/26/2015 09:52 PM, Jonathan Wakely wrote:

> Would changes like this be suitable for _FORTIFY_SOURCE?

> diff --git a/libstdc++-v3/include/std/mutex b/libstdc++-v3/include/std/mutex
> index 5e5ced1..074bf26 100644
> --- a/libstdc++-v3/include/std/mutex
> +++ b/libstdc++-v3/include/std/mutex
> @@ -70,7 +70,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>      __recursive_mutex_base& operator=(const __recursive_mutex_base&) = delete;
>  
>  #ifdef __GTHREAD_RECURSIVE_MUTEX_INIT
> +# if _GLIBCXX_ASSERTIONS && defined(PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP)
> +    // Use an error-checking mutex type when assertions are enabled.
> +    __native_type  _M_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
> +# else
>      __native_type  _M_mutex = __GTHREAD_RECURSIVE_MUTEX_INIT;
> +# endif

I think this is incorrect.

If you try to lock an error-checking mutex recursively, the operation
fails, and it does *not* increment the internal lock counter (the mutex
may not even have one).  This means a subsequent unlock operation will
release the mutex too early.

The trylock will be have differently, too.

POSIX recursive mutexes are already error-checking in that sense
(self-deadlock cannot happen, and an unlock when not lock is defined to
return an error), so I don't think anything like that is even needed.
diff mbox

Patch

diff --git a/libstdc++-v3/include/std/mutex b/libstdc++-v3/include/std/mutex
index 5e5ced1..074bf26 100644
--- a/libstdc++-v3/include/std/mutex
+++ b/libstdc++-v3/include/std/mutex
@@ -70,7 +70,12 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     __recursive_mutex_base& operator=(const __recursive_mutex_base&) = delete;
 
 #ifdef __GTHREAD_RECURSIVE_MUTEX_INIT
+# if _GLIBCXX_ASSERTIONS && defined(PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP)
+    // Use an error-checking mutex type when assertions are enabled.
+    __native_type  _M_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
+# else
     __native_type  _M_mutex = __GTHREAD_RECURSIVE_MUTEX_INIT;
+# endif
 
     __recursive_mutex_base() = default;
 #else