Re: [PATCH v2 2/4] Consolidate stdio-lock.h

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 27/04/2022 15:00, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 27/04/2022 10:25, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> diff --git a/sysdeps/generic/stdio-lock.h b/sysdeps/generic/stdio-lock.h
>>>> index 14cf458bdd..fd61f0b5b7 100644
>>>> --- a/sysdeps/generic/stdio-lock.h
>>>> +++ b/sysdeps/generic/stdio-lock.h
>>>> @@ -45,20 +45,13 @@ __libc_lock_define_recursive (typedef, _IO_lock_t)
>>>>  #define _IO_cleanup_region_end(_doit) \
>>>>    __libc_cleanup_region_end (_doit)
>>>>  
>>>> -#if defined _LIBC && IS_IN (libc)
>>>> -
>>>> -# ifdef __EXCEPTIONS
>>>> -# define _IO_acquire_lock(_fp) \
>>>> +#define _IO_acquire_lock(_fp) \
>>>>    do {									      \
>>>> -    FILE *_IO_acquire_lock_file						      \
>>>> -	__attribute__((cleanup (_IO_acquire_lock_fct)))			      \
>>>> -	= (_fp);							      \
>>>> -    _IO_flockfile (_IO_acquire_lock_file);
>>>> -# else
>>>> -#  define _IO_acquire_lock(_fp) _IO_acquire_lock_needs_exceptions_enabled
>>>> -# endif
>>>> -# define _IO_release_lock(_fp) ; } while (0)
>>>> -
>>>> -#endif
>>>> +    _IO_cleanup_region_start((void (*) (void *)) &_IO_funlockfile, _fp);      \
>>>> +    _IO_flockfile (_fp);
>>>> +#define _IO_release_lock(_fp) \
>>>> +    _IO_funlockfile (_fp);						      \
>>>> +    _IO_cleanup_region_end (0);						      \
>>>> +  } while (0)
>>>>  
>>>>  #endif /* stdio-lock.h */
>>>
>>> I think this change replaces unwind tables for -fexceptions builds.  If
>>> GCC can't turn the indirect call to the unlock function into a direct
>>> call, this will result in a loss of hardening due to the additional
>>> indirect function call.
>>>
>>> This change may also lose C++ unwinding compatibility for some
>>> fopencookie use cases, I think.
>>
>> This is an internal header where if __EXCEPTIONS is not defined we will
>> get a compiler error because of an undefined symbol
>> (_IO_acquire_lock_needs_exceptions_enabled).  So internally all
>> _IO_acquire_lock usage already requires __EXCEPTIONS, so the fallback
>> is just unused definitions.
> 
> I see this code generation change in libio/fputc.os.  The new code uses
> an on-stack pointer saved at the start of the cleanup region:
> 
> +  90:  48 8b 05 00 00 00 00    mov    0x0(%rip),%rax        # 97 <fputc+0x97>
> +                       93: R_X86_64_REX_GOTPCRELX      _IO_funlockfile-0x4
> +  97:  48 89 e7                mov    %rsp,%rdi
> +  9a:  48 89 04 24             mov    %rax,(%rsp)
> +  9e:  e8 00 00 00 00          call   a3 <fputc+0xa3>
> +                       9f: R_X86_64_PLT32      __GI___libc_cleanup_push_defer-0x4
> +  a3:  8b 45 00                mov    0x0(%rbp),%eax
> +  a6:  25 00 80 00 00          and    $0x8000,%eax
> +  ab:  0f 85 d1 00 00 00       jne    182 <fputc+0x182>
> +  b1:  64 4c 8b 2c 25 10 00    mov    %fs:0x10,%r13
> +  b8:  00 00 
> +  ba:  48 8b bd 88 00 00 00    mov    0x88(%rbp),%rdi
> +  c1:  4c 39 6f 08             cmp    %r13,0x8(%rdi)
> +  c5:  74 1a                   je     e1 <fputc+0xe1>
> +  c7:  ba 01 00 00 00          mov    $0x1,%edx
> +  cc:  f0 0f b1 17             lock cmpxchg %edx,(%rdi)
> +  d0:  0f 85 a2 00 00 00       jne    178 <fputc+0x178>
> +  d6:  48 8b bd 88 00 00 00    mov    0x88(%rbp),%rdi
> +  dd:  4c 89 6f 08             mov    %r13,0x8(%rdi)
> +  e1:  83 47 04 01             addl   $0x1,0x4(%rdi)
> +  e5:  41 bd 01 00 00 00       mov    $0x1,%r13d
> +  eb:  e9 3d ff ff ff          jmp    2d <fputc+0x2d>
> +  f0:  48 89 e7                mov    %rsp,%rdi
> +  f3:  e8 00 00 00 00          call   f8 <fputc+0xf8>
> +                       f4: R_X86_64_PLT32      __GI___libc_cleanup_pop_restore-0x4
> 
> This is a from a build with CFLAGS="-O2 -fexceptions -s -DNDEBUG"
> (for comparison purposes).
> 
> The old code just inlined the _IO_funlockfile fast path.
> 
> (We seem to lack libc_hidden_proto/libc_hidden_def for _IO_funlockfile.)

You are right.  The change now uses __libc_cleanup_region_start from libc-lock.h
instead of the cleanup version.  I am not sure about hardening, but afaiu 
__libc_cleanup_push_defer should handle C++ unwinding for fopencookie.

It seems that the Hurd version is indeed to the best option, so I think for
generic it would be better to just use:

diff --git a/sysdeps/generic/stdio-lock.h b/sysdeps/generic/stdio-lock.h
index 14cf458bdd..a42131f5a5 100644
--- a/sysdeps/generic/stdio-lock.h
+++ b/sysdeps/generic/stdio-lock.h
@@ -45,20 +45,12 @@ __libc_lock_define_recursive (typedef, _IO_lock_t)
 #define _IO_cleanup_region_end(_doit) \
   __libc_cleanup_region_end (_doit)

-#if defined _LIBC && IS_IN (libc)
-
-# ifdef __EXCEPTIONS
-# define _IO_acquire_lock(_fp) \
+#define _IO_acquire_lock(_fp) \
   do {                                                                       \
     FILE *_IO_acquire_lock_file                                                      \
        __attribute__((cleanup (_IO_acquire_lock_fct)))                       \
        = (_fp);                                                              \
     _IO_flockfile (_IO_acquire_lock_file);
-# else
-#  define _IO_acquire_lock(_fp) _IO_acquire_lock_needs_exceptions_enabled
-# endif
-# define _IO_release_lock(_fp) ; } while (0)
-
-#endif
+#define _IO_release_lock(_fp) ; } while (0)

 #endif /* stdio-lock.h */