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

[Florian Weimer <fweimer@redhat.com>]

* 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.)

Thanks,
Florian