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

[Cupertino Miranda <cupertino.miranda@oracle.com>]

Hi Carlos, Florian,

Last patchwork review meeting you mentioned some improvement patches to
(I think) __libc_cleanup_region_start/end mechanism.
I tried to find the patches in the libc-alpha archives without any
success.

Only one I found was something from 2018 changing it for mach:
https://ecos.sourceware.org/ml/libc-alpha/2018-03/msg00433.html

In any case the solution would not address the problem.

Can you possibly point me to the patches you refered to?
My searching ability is limited to the archives / google. It might be
simpler for you to find it. ;-)

Regards,
Cupertino

Cupertino Miranda via Libc-alpha writes:

> 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.
>
> I have some concerns about the original presented patch as, IMO, it would allow
> any user level application to bypass the lock/unlock by directly writing to
> the file pointer flags, changing the original definition of f[unlock|lock]file
> as described in the man pages.
>
> Should both the macro/function definitions be coherent such that they should be
> used interchangeably in glibc code?
>
> Regards,
> Cupertino
>
> --- MORE TARGETED PATCH ---
>
> diff --git a/stdio-common/vfscanf-internal.c b/stdio-common/vfscanf-internal.c
> index 2ad34050f3..5b55db962b 100644
> --- a/stdio-common/vfscanf-internal.c
> +++ b/stdio-common/vfscanf-internal.c
> @@ -177,11 +177,15 @@
>           return EOF;                                                         \
>         }                                                                     \
>      } while (0)
> +
> +static inline void checked_IO_funlockfile(FILE *s);
> +static inline void checked_IO_flockfile(FILE *s);
> +
>  #define LOCK_STREAM(S)                                                       \
> -  __libc_cleanup_region_start (1, (void (*) (void *)) &_IO_funlockfile, (S)); \
> -  _IO_flockfile (S)
> +  __libc_cleanup_region_start (1, (void (*) (void *)) &checked_IO_funlockfile, (S)); \
> +  checked_IO_flockfile (S)
>  #define UNLOCK_STREAM(S)                                                     \
> -  _IO_funlockfile (S);                                                       \
> +  checked_IO_funlockfile (S);                                                \
>    __libc_cleanup_region_end (0)
>
>  struct ptrs_to_free
> @@ -197,6 +201,18 @@ struct char_buffer {
>    struct scratch_buffer scratch;
>  };
>
> +static inline void
> +checked_IO_funlockfile(FILE *s)
> +{
> +  _IO_funlockfile (s);
> +}
> +
> +static inline void
> +checked_IO_flockfile(FILE *s)
> +{
> +  _IO_flockfile (s);
> +}
> +
>  /* Returns a pointer to the first CHAR_T object in the buffer.  Only
>     valid if char_buffer_add (BUFFER, CH) has been called and
>     char_buffer_error (BUFFER) is false.  */
>
>> From a0291671db0d92a8762de3a45fd322e471a19a24 Mon Sep 17 00:00:00 2001
>> From: Patrick McGehearty <patrick.mcgehearty@oracle.com>
>> Date: Wed, 23 Nov 2022 21:02:02 +0000
>> Subject: [PATCH v2] Resolve flockfile/funlockfile differences
>>
>> - - - - - -
>> Only difference from v1 is to correct indenting/tabs
>> for the changes to match other .c files in stdio-common.
>> - - - - - -
>> This patch resolves differences between flockfile/funlockfile and
>> LOCK_STREAM/UNLOCKSTREAM to avoid failure to unlock a stream mutex in
>> a multi-threaded application context which allows entering using
>> LOCK_STREAM but leaving using funlockfile or vice-versa.
>> This issue was detected during stress tests of a large proprietary
>> application. The cause and solution was identified by Gerd Rausch.
>>
>> The issue occurs because _IO_funlockfile has different definitions in
>> different contexts:
>>
>> Comparing the inline version in libio/libio.h
>> #  define _IO_flockfile(_fp) \
>>   if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_flockfile (_fp)
>>
>> And the non-inline version in stdio-common/flockfile.c
>> __flockfile (FILE *stream)
>> {
>>   _IO_lock_lock (*stream->_lock);
>> }
>>
>> Note the lack of the _IO_USER_LOCK in the __flockfile version.  This
>> difference means it is possible to bypass the lock in some cases and
>> not release the lock in other cases. Either way, it causes trouble.
>>
>> The proposed fix is to simple add the _IO_USER_LOCK guard to the
>> non-line versions of flockfile and funlockfile.
>>
>> Modified files:
>>       stdio-common/flockfile.c
>>       stdio-common/funlockfile.c
>> ---
>>  stdio-common/flockfile.c   | 3 ++-
>>  stdio-common/funlockfile.c | 3 ++-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/stdio-common/flockfile.c b/stdio-common/flockfile.c
>> index 49f72c6..7c82847 100644
>> --- a/stdio-common/flockfile.c
>> +++ b/stdio-common/flockfile.c
>> @@ -22,7 +22,8 @@
>>  void
>>  __flockfile (FILE *stream)
>>  {
>> -  _IO_lock_lock (*stream->_lock);
>> +  if ((stream->_flags & _IO_USER_LOCK) == 0)
>> +    _IO_lock_lock (*stream->_lock);
>>  }
>>  weak_alias (__flockfile, flockfile);
>>  weak_alias (__flockfile, _IO_flockfile)
>> diff --git a/stdio-common/funlockfile.c b/stdio-common/funlockfile.c
>> index bf44c99..b77b1b2 100644
>> --- a/stdio-common/funlockfile.c
>> +++ b/stdio-common/funlockfile.c
>> @@ -23,7 +23,8 @@
>>  void
>>  __funlockfile (FILE *stream)
>>  {
>> -  _IO_lock_unlock (*stream->_lock);
>> +  if ((stream->_flags & _IO_USER_LOCK) == 0)
>> +    _IO_lock_unlock (*stream->_lock);
>>  }
>>  weak_alias (__funlockfile, _IO_funlockfile)
>>  weak_alias (__funlockfile, funlockfile);
>> --
>> 1.8.3.1