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

[Cupertino Miranda]

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