Re: [PING] [PATCH v2] Resolve-flockfile-funlockfile-differences

[Florian Weimer <fweimer@redhat.com>]

* Cupertino Miranda via Libc-alpha:

> This is a request for review of the patch sent by Patrick McGehearty
> based on an inconsistency in flockfile and funlockfile definitions
> (macro/function) when used in glibc code.  At the same time this is a
> request for comments on the topic based on some personal concerns on
> the patch.
>
> After reading the description of the problem as explained in our bug
> tracking I decided to make a more targeted patch, which will also help
> to better explain the problem.
>
> Unfortunately at this stage I cannot functionally verify this patch
> due to how long it was identified and fixed at Oracle.
>
> The patch defines a local wrapper to flockfile and funlockfile such
> that both the function pointer passed to libc_cleanup_region_start and
> the direct call would point to the same definition.

So here's the general concern I have regarding
_IO_flockfile.  In libio/libio.h, we have:

    197 extern void _IO_flockfile (FILE *) __THROW;
    198 extern void _IO_funlockfile (FILE *) __THROW;
    199 extern int _IO_ftrylockfile (FILE *) __THROW;
    200 
    201 #define _IO_peekc(_fp) _IO_peekc_unlocked (_fp)
    202 #define _IO_flockfile(_fp) /**/
    203 #define _IO_funlockfile(_fp) ((void) 0)
    204 #define _IO_ftrylockfile(_fp) /**/
    205 #ifndef _IO_cleanup_region_start
    206 #define _IO_cleanup_region_start(_fct, _fp) /**/
    207 #endif
    208 #ifndef _IO_cleanup_region_end
    209 #define _IO_cleanup_region_end(_Doit) /**/
    210 #endif

    275 #ifdef _IO_MTSAFE_IO
    276 # undef _IO_peekc
    277 # undef _IO_flockfile
    278 # undef _IO_funlockfile
    279 # undef _IO_ftrylockfile
    280 
    281 # define _IO_peekc(_fp) _IO_peekc_locked (_fp)
    282 # define _IO_flockfile(_fp) \
    283   if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_lock_lock (*(_fp)->_lock    283 )
    284 # define _IO_funlockfile(_fp) \
    285   if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_lock_unlock (*(_fp)->_lo    285 ck)
    286 #endif /* _IO_MTSAFE_IO */

As you can see, the exact definition depends on _IO_MTSAFE_IO.  It is
set in Makeconfig, but again conditionally:

   1397 # A sysdeps Makeconfig fragment may set libc-reentrant to yes.
   1398 ifeq (yes,$(libc-reentrant))
   1399 defines += -D_LIBC_REENTRANT
   1400 
   1401 libio-mtsafe = -D_IO_MTSAFE_IO
   1402 endif

$(libc-reentrant) should always evaluate to yes because it is set as
such in sysdeps/pthread/Makeconfig.

On the other hand, this construct requires further opt-in by including
libio-mtsafe in CFLAGS or CPPFLAGS.  There seem to be uses that are
appear to be unconvered by -D_IO_MTSAFE_IO, for example the malloc
subdirectory.  Disassembling __malloc_stats seems to confirm that.

Maybe you can confirm based on your case notes whether this issue might
becaused by parts of glibc stomping over the flags due to the
inconsistency described above.  Or is it because the application sets
__fsetlocking (FSETLOCKING_BYCALLER) concurrently?  I don't see how we
can support the latter, that really seems to be an application bug.

Thanks,
Florian