Patchwork libio: Eliminate _IO_stdin, _IO_stdout, _IO_stderr

login
register
mail settings
Submitter Florian Weimer
Date Feb. 3, 2019, 10:04 a.m.
Message ID <87lg2x5gsf.fsf@oldenburg2.str.redhat.com>
Download mbox | patch
Permalink /patch/31288/
State Superseded
Headers show

Comments

Florian Weimer - Feb. 3, 2019, 10:04 a.m.
These variables are only used to determine if a stdio stream is
a pre-allocated stream, but it is possible to do so by comparing
a FILE * to all pre-allocated stream objects.  As a result, it is
not necessary to keep those pointers in separate variables.

Behavior with symbol interposition is unchanged because _IO_stdin_,
_IO_stdout_, _IO_stderr_ are exported, and refer to objects outside of
libc if symbol interposition or copy relocations are involved.

2019-02-03  Florian Weimer  <fweimer@redhat.com>

	* libio/libio.h (_IO_stdin, _IO_stdout, _IO_stderr): Remove
	declaration.
	* libio/stdio.c (AL, AL2, _IO_stdin, _IO_stdout, _IO_stderr):
	Remove definitions.
	* libio/stdfiles.c: Update comment.
	* libio/oldstdfiles.c (_IO_check_libio): Update comment.  Do not
	set _IO_stdin, _IO_stdout, _IO_stderr.
	* libio/libioP.h (_IO_fake_stdiobuf): Remove unused declaration.
	[SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)] (_IO_legacy_file): New
	inline function.
	(_IO_deallocate_file): New inline function.
	* libio/iolibio.h (_IO_vprintf): Remove definition.
	* libio/iofclose.c (_IO_new_fclose): Use _IO_deallocate_file.
	* libio/oldiofclose.c (_IO_old_fclose): Likewise.
	* libio/iofwide.c (_IO_fwide): Use __glibc_unlikely and
	_IO_legacy_file.
	* libio/oldfileops.c (_IO_old_file_init_internal): Remove
	__builtin_expect.  Use _IO_legacy_file.
Florian Weimer - Feb. 12, 2019, 2:18 p.m.
* Florian Weimer:

> These variables are only used to determine if a stdio stream is
> a pre-allocated stream, but it is possible to do so by comparing
> a FILE * to all pre-allocated stream objects.  As a result, it is
> not necessary to keep those pointers in separate variables.
>
> Behavior with symbol interposition is unchanged because _IO_stdin_,
> _IO_stdout_, _IO_stderr_ are exported, and refer to objects outside of
> libc if symbol interposition or copy relocations are involved.
>
> 2019-02-03  Florian Weimer  <fweimer@redhat.com>
>
> 	* libio/libio.h (_IO_stdin, _IO_stdout, _IO_stderr): Remove
> 	declaration.
> 	* libio/stdio.c (AL, AL2, _IO_stdin, _IO_stdout, _IO_stderr):
> 	Remove definitions.
> 	* libio/stdfiles.c: Update comment.
> 	* libio/oldstdfiles.c (_IO_check_libio): Update comment.  Do not
> 	set _IO_stdin, _IO_stdout, _IO_stderr.
> 	* libio/libioP.h (_IO_fake_stdiobuf): Remove unused declaration.
> 	[SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)] (_IO_legacy_file): New
> 	inline function.
> 	(_IO_deallocate_file): New inline function.
> 	* libio/iolibio.h (_IO_vprintf): Remove definition.
> 	* libio/iofclose.c (_IO_new_fclose): Use _IO_deallocate_file.
> 	* libio/oldiofclose.c (_IO_old_fclose): Likewise.
> 	* libio/iofwide.c (_IO_fwide): Use __glibc_unlikely and
> 	_IO_legacy_file.
> 	* libio/oldfileops.c (_IO_old_file_init_internal): Remove
> 	__builtin_expect.  Use _IO_legacy_file.

Ping?  <https://sourceware.org/ml/libc-alpha/2019-02/msg00057.html>

Thanks,
Florian
Gabriel F. T. Gomes - Feb. 12, 2019, 4:36 p.m.
Hi, Florian.

I have a question about the change in libio/oldfileops.c (see below).
The rest of the patch looks good to me.

On Sun, Feb 03 2019, Florian Weimer wrote:
> These variables are only used to determine if a stdio stream is
> a pre-allocated stream, but it is possible to do so by comparing
> a FILE * to all pre-allocated stream objects.  As a result, it is
> not necessary to keep those pointers in separate variables.
> 
> Behavior with symbol interposition is unchanged because _IO_stdin_,
> _IO_stdout_, _IO_stderr_ are exported, and refer to objects outside of
> libc if symbol interposition or copy relocations are involved.
> 
> 2019-02-03  Florian Weimer  <fweimer@redhat.com>
> 
> 	* libio/libio.h (_IO_stdin, _IO_stdout, _IO_stderr): Remove
> 	declaration.
> 	* libio/stdio.c (AL, AL2, _IO_stdin, _IO_stdout, _IO_stderr):
> 	Remove definitions.
> 	* libio/stdfiles.c: Update comment.
> 	* libio/oldstdfiles.c (_IO_check_libio): Update comment.  Do not
> 	set _IO_stdin, _IO_stdout, _IO_stderr.
> 	* libio/libioP.h (_IO_fake_stdiobuf): Remove unused declaration.
> 	[SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)] (_IO_legacy_file): New
> 	inline function.
> 	(_IO_deallocate_file): New inline function.
> 	* libio/iolibio.h (_IO_vprintf): Remove definition.
> 	* libio/iofclose.c (_IO_new_fclose): Use _IO_deallocate_file.
> 	* libio/oldiofclose.c (_IO_old_fclose): Likewise.
> 	* libio/iofwide.c (_IO_fwide): Use __glibc_unlikely and
> 	_IO_legacy_file.
> 	* libio/oldfileops.c (_IO_old_file_init_internal): Remove
> 	__builtin_expect.  Use _IO_legacy_file.
> 
> diff --git a/libio/iofclose.c b/libio/iofclose.c
> index 9b39a6cc4e..8a80dd0b78 100644
> --- a/libio/iofclose.c
> +++ b/libio/iofclose.c
> @@ -71,12 +71,7 @@ _IO_new_fclose (FILE *fp)
>        if (_IO_have_backup (fp))
>  	_IO_free_backup_area (fp);
>      }
> -  if (fp != _IO_stdin && fp != _IO_stdout && fp != _IO_stderr)
> -    {
> -      fp->_flags = 0;
> -      free(fp);
> -    }
> -
> +  _IO_deallocate_file (fp);

OK.

>    return status;
>  }
>  
> diff --git a/libio/iofwide.c b/libio/iofwide.c
> index 6676ad5e53..247cfde3d0 100644
> --- a/libio/iofwide.c
> +++ b/libio/iofwide.c
> @@ -87,8 +87,7 @@ _IO_fwide (FILE *fp, int mode)
>    mode = mode < 0 ? -1 : (mode == 0 ? 0 : 1);
>  
>  #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
> -  if (__builtin_expect (&_IO_stdin_used == NULL, 0)
> -      && (fp == _IO_stdin || fp == _IO_stdout || fp == _IO_stderr))
> +  if (__glibc_unlikely (&_IO_stdin_used == NULL) && _IO_legacy_file (fp))

OK.

>      /* This is for a stream in the glibc 2.0 format.  */
>      return -1;
>  #endif
> diff --git a/libio/iolibio.h b/libio/iolibio.h
> index 2642d71e4f..9561833655 100644
> --- a/libio/iolibio.h
> +++ b/libio/iolibio.h
> @@ -58,8 +58,6 @@ extern int _IO_vsscanf (const char *, const char *, __gnuc_va_list) __THROW;
>     == _IO_pos_BAD ? EOF : 0)
>  #define _IO_rewind(FILE) \
>    (void) _IO_seekoff_unlocked (FILE, 0, 0, _IOS_INPUT|_IOS_OUTPUT)
> -#define _IO_vprintf(FORMAT, ARGS) \
> -  _IO_vfprintf (_IO_stdout, FORMAT, ARGS)
>  #define _IO_freopen(FILENAME, MODE, FP) \
>    (_IO_file_close_it (FP), \
>     _IO_file_fopen (FP, FILENAME, MODE, 1))
> diff --git a/libio/libio.h b/libio/libio.h
> index d21162aab0..872574cfb7 100644
> --- a/libio/libio.h
> +++ b/libio/libio.h
> @@ -185,9 +185,6 @@ struct _IO_FILE_plus;
>  extern struct _IO_FILE_plus _IO_2_1_stdin_;
>  extern struct _IO_FILE_plus _IO_2_1_stdout_;
>  extern struct _IO_FILE_plus _IO_2_1_stderr_;
> -extern FILE *_IO_stdin attribute_hidden;
> -extern FILE *_IO_stdout attribute_hidden;
> -extern FILE *_IO_stderr attribute_hidden;

The removal.  OK.

>  
>  struct _IO_cookie_file;
>  
> diff --git a/libio/libioP.h b/libio/libioP.h
> index 8c75f15167..1c434ec3a1 100644
> --- a/libio/libioP.h
> +++ b/libio/libioP.h
> @@ -817,7 +817,35 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
>  # endif
>  #endif
>  
> -extern struct _IO_fake_stdiobuf _IO_stdin_buf, _IO_stdout_buf, _IO_stderr_buf;
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
> +/* See oldstdfiles.c.  These are the old stream variables.  */
> +extern struct _IO_FILE_plus _IO_stdin_;
> +extern struct _IO_FILE_plus _IO_stdout_;
> +extern struct _IO_FILE_plus _IO_stderr_;
> +
> +static inline bool
> +_IO_legacy_file (FILE *fp)
> +{
> +  return fp == (FILE *) &_IO_stdin_ || fp == (FILE *) &_IO_stdout_
> +    || fp == (FILE *) &_IO_stderr_;
> +}
> +#endif
> +
> +/* Deallocate a stream if it is heap-allocated.  Preallocated
> +   stdin/stdout/stderr streams are not deallocated. */
> +static inline void
> +_IO_deallocate_file (FILE *fp)
> +{
> +  /* The current stream variables.  */
> +  if (fp == (FILE *) &_IO_2_1_stdin_ || fp == (FILE *) &_IO_2_1_stdout_
> +      || fp == (FILE *) &_IO_2_1_stderr_)
> +    return;
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
> +  if (_IO_legacy_file (fp))
> +    return;
> +#endif
> +  free (fp);
> +}

OK.

>  
>  #ifdef IO_DEBUG
>  # define CHECK_FILE(FILE, RET) do {			\
> diff --git a/libio/oldfileops.c b/libio/oldfileops.c
> index 4d6c5e3fe7..10f2205e8a 100644
> --- a/libio/oldfileops.c
> +++ b/libio/oldfileops.c
> @@ -109,10 +109,7 @@ _IO_old_file_init_internal (struct _IO_FILE_plus *fp)
>  			     - (int) sizeof (struct _IO_FILE_complete));
>    fp->file._fileno = -1;
>  
> -  if (__builtin_expect (&_IO_stdin_used != NULL, 1)
> -      || (fp != (struct _IO_FILE_plus *) _IO_stdin
> -	  && fp != (struct _IO_FILE_plus *) _IO_stdout
> -	  && fp != (struct _IO_FILE_plus *) _IO_stderr))
> +  if (&_IO_stdin_used != NULL && !_IO_legacy_file ((FILE *) fp))

Why did the `||' become an `&&'?  Is this solving a bug?

>      /* The object is dynamically allocated and large enough.  Initialize
>         the _mode element as well.  */
>      ((struct _IO_FILE_complete *) fp)->_mode = -1;
> diff --git a/libio/oldiofclose.c b/libio/oldiofclose.c
> index e4cbf88566..be5044c3bd 100644
> --- a/libio/oldiofclose.c
> +++ b/libio/oldiofclose.c
> @@ -58,12 +58,7 @@ _IO_old_fclose (FILE *fp)
>    _IO_FINISH (fp);
>    if (_IO_have_backup (fp))
>      _IO_free_backup_area (fp);
> -  if (fp != _IO_stdin && fp != _IO_stdout && fp != _IO_stderr)
> -    {
> -      fp->_flags = 0;
> -      free(fp);
> -    }
> -
> +  _IO_deallocate_file (fp);

OK.

>    return status;
>  }
>  
> diff --git a/libio/oldstdfiles.c b/libio/oldstdfiles.c
> index 524e260b7e..2b861cd754 100644
> --- a/libio/oldstdfiles.c
> +++ b/libio/oldstdfiles.c
> @@ -27,11 +27,8 @@
>  #include <shlib-compat.h>
>  #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
>  
> -/* This file provides definitions of _IO_stdin, _IO_stdout, and _IO_stderr
> -   for C code.  Compare stdstreams.cc.
> -   (The difference is that here the vtable field is set to 0,
> -   so the objects defined are not valid C++ objects.  On the other
> -   hand, we don't need a C++ compiler to build this file.) */
> +/* This file provides legacy definitions of _IO_stdin_, _IO_stdout_,
> +   and _IO_stderr_.  See stdfiles.c for the current definitions.  */
>  
>  #define _IO_USE_OLD_IO_FILE
>  #include "libioP.h"
> @@ -78,13 +75,12 @@ _IO_check_libio (void)
>    if (&_IO_stdin_used == NULL)
>      {
>        /* We are using the old one. */
> -      _IO_stdin = stdin = (FILE *) &_IO_stdin_;
> -      _IO_stdout = stdout = (FILE *) &_IO_stdout_;
> -      _IO_stderr = stderr = (FILE *) &_IO_stderr_;
> +      stdin = (FILE *) &_IO_stdin_;
> +      stdout = (FILE *) &_IO_stdout_;
> +      stderr = (FILE *) &_IO_stderr_;

_IO_std* are gone.  OK.

>        _IO_list_all = &_IO_stderr_;
> -      _IO_stdin->_vtable_offset = _IO_stdout->_vtable_offset =
> -	_IO_stderr->_vtable_offset = stdin->_vtable_offset =
> -	stdout->_vtable_offset = stderr->_vtable_offset =
> +      stdin->_vtable_offset = stdout->_vtable_offset
> +	= stderr->_vtable_offset =

Likewise.  OK.

>  	((int) sizeof (struct _IO_FILE)
>  	 - (int) sizeof (struct _IO_FILE_complete));
>      }
> diff --git a/libio/stdfiles.c b/libio/stdfiles.c
> index 605e006474..9c779b47eb 100644
> --- a/libio/stdfiles.c
> +++ b/libio/stdfiles.c
> @@ -25,11 +25,10 @@
>     in files containing the exception.  */
>  
>  
> -/* This file provides definitions of _IO_stdin, _IO_stdout, and _IO_stderr
> -   for C code.  Compare stdstreams.cc.
> -   (The difference is that here the vtable field is set to 0,
> -   so the objects defined are not valid C++ objects.  On the other
> -   hand, we don't need a C++ compiler to build this file.) */
> +/* This file provides definitions of _IO_2_1_stdin_, _IO_2_1_stdout_,
> +   and _IO_2_1_stderr_, the default values of stdin, stdout, stderr.
> +   See oldstdfiles.c for glibc 2.0 legacy definitions without wide
> +   character support.  */
>  
>  #include "libioP.h"
>  
> diff --git a/libio/stdio.c b/libio/stdio.c
> index 1294d2842e..522de44a27 100644
> --- a/libio/stdio.c
> +++ b/libio/stdio.c
> @@ -33,14 +33,3 @@
>  FILE *stdin = (FILE *) &_IO_2_1_stdin_;
>  FILE *stdout = (FILE *) &_IO_2_1_stdout_;
>  FILE *stderr = (FILE *) &_IO_2_1_stderr_;
> -
> -#undef _IO_stdin
> -#undef _IO_stdout
> -#undef _IO_stderr
> -#define AL(name) AL2 (name, _IO_##name)
> -#define AL2(name, al) \
> -  extern __typeof (name) al __attribute__ ((alias (#name),                    \
> -                                            visibility ("hidden")))
> -AL(stdin);
> -AL(stdout);
> -AL(stderr);

Gone.  OK.
Dmitry Levin - Feb. 13, 2019, 12:51 a.m.
Hi,

On Sun, Feb 03, 2019 at 11:04:48AM +0100, Florian Weimer wrote:
> These variables are only used to determine if a stdio stream is
> a pre-allocated stream, but it is possible to do so by comparing
> a FILE * to all pre-allocated stream objects.  As a result, it is
> not necessary to keep those pointers in separate variables.
> 
> Behavior with symbol interposition is unchanged because _IO_stdin_,
> _IO_stdout_, _IO_stderr_ are exported, and refer to objects outside of
> libc if symbol interposition or copy relocations are involved.
> 
> 2019-02-03  Florian Weimer  <fweimer@redhat.com>
> 
> 	* libio/libio.h (_IO_stdin, _IO_stdout, _IO_stderr): Remove
> 	declaration.
> 	* libio/stdio.c (AL, AL2, _IO_stdin, _IO_stdout, _IO_stderr):
> 	Remove definitions.
> 	* libio/stdfiles.c: Update comment.
> 	* libio/oldstdfiles.c (_IO_check_libio): Update comment.  Do not
> 	set _IO_stdin, _IO_stdout, _IO_stderr.
> 	* libio/libioP.h (_IO_fake_stdiobuf): Remove unused declaration.
> 	[SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)] (_IO_legacy_file): New
> 	inline function.
> 	(_IO_deallocate_file): New inline function.
> 	* libio/iolibio.h (_IO_vprintf): Remove definition.
> 	* libio/iofclose.c (_IO_new_fclose): Use _IO_deallocate_file.
> 	* libio/oldiofclose.c (_IO_old_fclose): Likewise.
> 	* libio/iofwide.c (_IO_fwide): Use __glibc_unlikely and
> 	_IO_legacy_file.
> 	* libio/oldfileops.c (_IO_old_file_init_internal): Remove
> 	__builtin_expect.  Use _IO_legacy_file.

Looks like _IO_legacy_file makes sense only when &_IO_stdin_used == NULL.
If the check was moved inside _IO_legacy_file, then ...

> diff --git a/libio/iofclose.c b/libio/iofclose.c
> index 9b39a6cc4e..8a80dd0b78 100644
> --- a/libio/iofclose.c
> +++ b/libio/iofclose.c
> @@ -71,12 +71,7 @@ _IO_new_fclose (FILE *fp)
>        if (_IO_have_backup (fp))
>  	_IO_free_backup_area (fp);
>      }
> -  if (fp != _IO_stdin && fp != _IO_stdout && fp != _IO_stderr)
> -    {
> -      fp->_flags = 0;
> -      free(fp);
> -    }
> -
> +  _IO_deallocate_file (fp);
>    return status;
>  }
>  
> diff --git a/libio/iofwide.c b/libio/iofwide.c
> index 6676ad5e53..247cfde3d0 100644
> --- a/libio/iofwide.c
> +++ b/libio/iofwide.c
> @@ -87,8 +87,7 @@ _IO_fwide (FILE *fp, int mode)
>    mode = mode < 0 ? -1 : (mode == 0 ? 0 : 1);
>  
>  #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
> -  if (__builtin_expect (&_IO_stdin_used == NULL, 0)
> -      && (fp == _IO_stdin || fp == _IO_stdout || fp == _IO_stderr))
> +  if (__glibc_unlikely (&_IO_stdin_used == NULL) && _IO_legacy_file (fp))

... this would become
	if (_IO_legacy_file (fp)))

>      /* This is for a stream in the glibc 2.0 format.  */
>      return -1;
>  #endif
> diff --git a/libio/iolibio.h b/libio/iolibio.h
> index 2642d71e4f..9561833655 100644
> --- a/libio/iolibio.h
> +++ b/libio/iolibio.h
> @@ -58,8 +58,6 @@ extern int _IO_vsscanf (const char *, const char *, __gnuc_va_list) __THROW;
>     == _IO_pos_BAD ? EOF : 0)
>  #define _IO_rewind(FILE) \
>    (void) _IO_seekoff_unlocked (FILE, 0, 0, _IOS_INPUT|_IOS_OUTPUT)
> -#define _IO_vprintf(FORMAT, ARGS) \
> -  _IO_vfprintf (_IO_stdout, FORMAT, ARGS)
>  #define _IO_freopen(FILENAME, MODE, FP) \
>    (_IO_file_close_it (FP), \
>     _IO_file_fopen (FP, FILENAME, MODE, 1))
> diff --git a/libio/libio.h b/libio/libio.h
> index d21162aab0..872574cfb7 100644
> --- a/libio/libio.h
> +++ b/libio/libio.h
> @@ -185,9 +185,6 @@ struct _IO_FILE_plus;
>  extern struct _IO_FILE_plus _IO_2_1_stdin_;
>  extern struct _IO_FILE_plus _IO_2_1_stdout_;
>  extern struct _IO_FILE_plus _IO_2_1_stderr_;
> -extern FILE *_IO_stdin attribute_hidden;
> -extern FILE *_IO_stdout attribute_hidden;
> -extern FILE *_IO_stderr attribute_hidden;
>  
>  struct _IO_cookie_file;
>  
> diff --git a/libio/libioP.h b/libio/libioP.h
> index 8c75f15167..1c434ec3a1 100644
> --- a/libio/libioP.h
> +++ b/libio/libioP.h
> @@ -817,7 +817,35 @@ extern int _IO_vscanf (const char *, va_list) __THROW;
>  # endif
>  #endif
>  
> -extern struct _IO_fake_stdiobuf _IO_stdin_buf, _IO_stdout_buf, _IO_stderr_buf;
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
> +/* See oldstdfiles.c.  These are the old stream variables.  */
> +extern struct _IO_FILE_plus _IO_stdin_;
> +extern struct _IO_FILE_plus _IO_stdout_;
> +extern struct _IO_FILE_plus _IO_stderr_;
> +
> +static inline bool
> +_IO_legacy_file (FILE *fp)
> +{
> +  return fp == (FILE *) &_IO_stdin_ || fp == (FILE *) &_IO_stdout_
> +    || fp == (FILE *) &_IO_stderr_;
> +}
> +#endif
> +
> +/* Deallocate a stream if it is heap-allocated.  Preallocated
> +   stdin/stdout/stderr streams are not deallocated. */
> +static inline void
> +_IO_deallocate_file (FILE *fp)
> +{
> +  /* The current stream variables.  */
> +  if (fp == (FILE *) &_IO_2_1_stdin_ || fp == (FILE *) &_IO_2_1_stdout_
> +      || fp == (FILE *) &_IO_2_1_stderr_)
> +    return;
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
> +  if (_IO_legacy_file (fp))

... this would save us from 3 extra checks for _IO_stdin_, _IO_stdout_,
and _IO_stderr_, and ...

> +    return;
> +#endif
> +  free (fp);
> +}
>  
>  #ifdef IO_DEBUG
>  # define CHECK_FILE(FILE, RET) do {			\
> diff --git a/libio/oldfileops.c b/libio/oldfileops.c
> index 4d6c5e3fe7..10f2205e8a 100644
> --- a/libio/oldfileops.c
> +++ b/libio/oldfileops.c
> @@ -109,10 +109,7 @@ _IO_old_file_init_internal (struct _IO_FILE_plus *fp)
>  			     - (int) sizeof (struct _IO_FILE_complete));
>    fp->file._fileno = -1;
>  
> -  if (__builtin_expect (&_IO_stdin_used != NULL, 1)
> -      || (fp != (struct _IO_FILE_plus *) _IO_stdin
> -	  && fp != (struct _IO_FILE_plus *) _IO_stdout
> -	  && fp != (struct _IO_FILE_plus *) _IO_stderr))
> +  if (&_IO_stdin_used != NULL && !_IO_legacy_file ((FILE *) fp))

... and this would become
	if (!_IO_legacy_file ((FILE *) fp))

Overall a bit cleaner and a bit less error-prone.
Florian Weimer - Feb. 18, 2019, 9:58 a.m.
* Dmitry V. Levin:

> Looks like _IO_legacy_file makes sense only when &_IO_stdin_used == NULL.
> If the check was moved inside _IO_legacy_file, then ...

I'm not sure.  We have seen cases where new binaries do not define
_IO_stdin_used, perhaps related to symbol visibility.

<https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=634261>
<https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=816059>
<https://bugs.launchpad.net/ubuntu/+source/lua5.3/+bug/1570055>

So at least for the free call (in _IO_deallocate_file after the patch),
I really want to check that the object isn't any of the preallocated
ones, in case the crashes aren't immediate and we have the potential
here for causing heap corruption.

Thanks,
Florian
Dmitry Levin - Feb. 18, 2019, 11:51 a.m.
On Mon, Feb 18, 2019 at 10:58:47AM +0100, Florian Weimer wrote:
> * Dmitry V. Levin:
> 
> > Looks like _IO_legacy_file makes sense only when &_IO_stdin_used == NULL.
> > If the check was moved inside _IO_legacy_file, then ...
> 
> I'm not sure.  We have seen cases where new binaries do not define
> _IO_stdin_used, perhaps related to symbol visibility.
> 
> <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=634261>
> <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=816059>
> <https://bugs.launchpad.net/ubuntu/+source/lua5.3/+bug/1570055>

Yes, and we also have
https://sourceware.org/bugzilla/show_bug.cgi?id=17908

> So at least for the free call (in _IO_deallocate_file after the patch),
> I really want to check that the object isn't any of the preallocated
> ones, in case the crashes aren't immediate and we have the potential
> here for causing heap corruption.

Fair enough.

Why do we check
	(&_IO_stdin_used == NULL) && _IO_legacy_file (fp)
instead of just
	_IO_legacy_file (fp)
then?  Is it just an optimization?
Florian Weimer - Feb. 18, 2019, 12:02 p.m.
* Dmitry V. Levin:

> On Mon, Feb 18, 2019 at 10:58:47AM +0100, Florian Weimer wrote:
>> * Dmitry V. Levin:
>> 
>> > Looks like _IO_legacy_file makes sense only when &_IO_stdin_used == NULL.
>> > If the check was moved inside _IO_legacy_file, then ...
>> 
>> I'm not sure.  We have seen cases where new binaries do not define
>> _IO_stdin_used, perhaps related to symbol visibility.
>> 
>> <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=634261>
>> <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=816059>
>> <https://bugs.launchpad.net/ubuntu/+source/lua5.3/+bug/1570055>
>
> Yes, and we also have
> https://sourceware.org/bugzilla/show_bug.cgi?id=17908
>
>> So at least for the free call (in _IO_deallocate_file after the patch),
>> I really want to check that the object isn't any of the preallocated
>> ones, in case the crashes aren't immediate and we have the potential
>> here for causing heap corruption.
>
> Fair enough.
>
> Why do we check
> 	(&_IO_stdin_used == NULL) && _IO_legacy_file (fp)
> instead of just
> 	_IO_legacy_file (fp)
> then?  Is it just an optimization?

Mainly to match the previous (buggy) implementation.

I think in _IO_fwide and _IO_old_file_init_internal, we could perform
the simplified check.  I've looked into that for my reply to Gabriel,
and the code assumes that we over-allocate even in the old
implementation, and only the objects that come from _IO_stdin_ etc. (the
three statically allocated FILE objects) are too small.

I don't think this assumption is entirely correct, and I'd prefer to
match the old behavior as close as possible here.  The reason for
eliminating _IO_stdin is a slight simplification of libio, not to fix
all those bugs.

Thanks,
Florian

Patch

diff --git a/libio/iofclose.c b/libio/iofclose.c
index 9b39a6cc4e..8a80dd0b78 100644
--- a/libio/iofclose.c
+++ b/libio/iofclose.c
@@ -71,12 +71,7 @@  _IO_new_fclose (FILE *fp)
       if (_IO_have_backup (fp))
 	_IO_free_backup_area (fp);
     }
-  if (fp != _IO_stdin && fp != _IO_stdout && fp != _IO_stderr)
-    {
-      fp->_flags = 0;
-      free(fp);
-    }
-
+  _IO_deallocate_file (fp);
   return status;
 }
 
diff --git a/libio/iofwide.c b/libio/iofwide.c
index 6676ad5e53..247cfde3d0 100644
--- a/libio/iofwide.c
+++ b/libio/iofwide.c
@@ -87,8 +87,7 @@  _IO_fwide (FILE *fp, int mode)
   mode = mode < 0 ? -1 : (mode == 0 ? 0 : 1);
 
 #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
-  if (__builtin_expect (&_IO_stdin_used == NULL, 0)
-      && (fp == _IO_stdin || fp == _IO_stdout || fp == _IO_stderr))
+  if (__glibc_unlikely (&_IO_stdin_used == NULL) && _IO_legacy_file (fp))
     /* This is for a stream in the glibc 2.0 format.  */
     return -1;
 #endif
diff --git a/libio/iolibio.h b/libio/iolibio.h
index 2642d71e4f..9561833655 100644
--- a/libio/iolibio.h
+++ b/libio/iolibio.h
@@ -58,8 +58,6 @@  extern int _IO_vsscanf (const char *, const char *, __gnuc_va_list) __THROW;
    == _IO_pos_BAD ? EOF : 0)
 #define _IO_rewind(FILE) \
   (void) _IO_seekoff_unlocked (FILE, 0, 0, _IOS_INPUT|_IOS_OUTPUT)
-#define _IO_vprintf(FORMAT, ARGS) \
-  _IO_vfprintf (_IO_stdout, FORMAT, ARGS)
 #define _IO_freopen(FILENAME, MODE, FP) \
   (_IO_file_close_it (FP), \
    _IO_file_fopen (FP, FILENAME, MODE, 1))
diff --git a/libio/libio.h b/libio/libio.h
index d21162aab0..872574cfb7 100644
--- a/libio/libio.h
+++ b/libio/libio.h
@@ -185,9 +185,6 @@  struct _IO_FILE_plus;
 extern struct _IO_FILE_plus _IO_2_1_stdin_;
 extern struct _IO_FILE_plus _IO_2_1_stdout_;
 extern struct _IO_FILE_plus _IO_2_1_stderr_;
-extern FILE *_IO_stdin attribute_hidden;
-extern FILE *_IO_stdout attribute_hidden;
-extern FILE *_IO_stderr attribute_hidden;
 
 struct _IO_cookie_file;
 
diff --git a/libio/libioP.h b/libio/libioP.h
index 8c75f15167..1c434ec3a1 100644
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -817,7 +817,35 @@  extern int _IO_vscanf (const char *, va_list) __THROW;
 # endif
 #endif
 
-extern struct _IO_fake_stdiobuf _IO_stdin_buf, _IO_stdout_buf, _IO_stderr_buf;
+#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
+/* See oldstdfiles.c.  These are the old stream variables.  */
+extern struct _IO_FILE_plus _IO_stdin_;
+extern struct _IO_FILE_plus _IO_stdout_;
+extern struct _IO_FILE_plus _IO_stderr_;
+
+static inline bool
+_IO_legacy_file (FILE *fp)
+{
+  return fp == (FILE *) &_IO_stdin_ || fp == (FILE *) &_IO_stdout_
+    || fp == (FILE *) &_IO_stderr_;
+}
+#endif
+
+/* Deallocate a stream if it is heap-allocated.  Preallocated
+   stdin/stdout/stderr streams are not deallocated. */
+static inline void
+_IO_deallocate_file (FILE *fp)
+{
+  /* The current stream variables.  */
+  if (fp == (FILE *) &_IO_2_1_stdin_ || fp == (FILE *) &_IO_2_1_stdout_
+      || fp == (FILE *) &_IO_2_1_stderr_)
+    return;
+#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
+  if (_IO_legacy_file (fp))
+    return;
+#endif
+  free (fp);
+}
 
 #ifdef IO_DEBUG
 # define CHECK_FILE(FILE, RET) do {			\
diff --git a/libio/oldfileops.c b/libio/oldfileops.c
index 4d6c5e3fe7..10f2205e8a 100644
--- a/libio/oldfileops.c
+++ b/libio/oldfileops.c
@@ -109,10 +109,7 @@  _IO_old_file_init_internal (struct _IO_FILE_plus *fp)
 			     - (int) sizeof (struct _IO_FILE_complete));
   fp->file._fileno = -1;
 
-  if (__builtin_expect (&_IO_stdin_used != NULL, 1)
-      || (fp != (struct _IO_FILE_plus *) _IO_stdin
-	  && fp != (struct _IO_FILE_plus *) _IO_stdout
-	  && fp != (struct _IO_FILE_plus *) _IO_stderr))
+  if (&_IO_stdin_used != NULL && !_IO_legacy_file ((FILE *) fp))
     /* The object is dynamically allocated and large enough.  Initialize
        the _mode element as well.  */
     ((struct _IO_FILE_complete *) fp)->_mode = -1;
diff --git a/libio/oldiofclose.c b/libio/oldiofclose.c
index e4cbf88566..be5044c3bd 100644
--- a/libio/oldiofclose.c
+++ b/libio/oldiofclose.c
@@ -58,12 +58,7 @@  _IO_old_fclose (FILE *fp)
   _IO_FINISH (fp);
   if (_IO_have_backup (fp))
     _IO_free_backup_area (fp);
-  if (fp != _IO_stdin && fp != _IO_stdout && fp != _IO_stderr)
-    {
-      fp->_flags = 0;
-      free(fp);
-    }
-
+  _IO_deallocate_file (fp);
   return status;
 }
 
diff --git a/libio/oldstdfiles.c b/libio/oldstdfiles.c
index 524e260b7e..2b861cd754 100644
--- a/libio/oldstdfiles.c
+++ b/libio/oldstdfiles.c
@@ -27,11 +27,8 @@ 
 #include <shlib-compat.h>
 #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
 
-/* This file provides definitions of _IO_stdin, _IO_stdout, and _IO_stderr
-   for C code.  Compare stdstreams.cc.
-   (The difference is that here the vtable field is set to 0,
-   so the objects defined are not valid C++ objects.  On the other
-   hand, we don't need a C++ compiler to build this file.) */
+/* This file provides legacy definitions of _IO_stdin_, _IO_stdout_,
+   and _IO_stderr_.  See stdfiles.c for the current definitions.  */
 
 #define _IO_USE_OLD_IO_FILE
 #include "libioP.h"
@@ -78,13 +75,12 @@  _IO_check_libio (void)
   if (&_IO_stdin_used == NULL)
     {
       /* We are using the old one. */
-      _IO_stdin = stdin = (FILE *) &_IO_stdin_;
-      _IO_stdout = stdout = (FILE *) &_IO_stdout_;
-      _IO_stderr = stderr = (FILE *) &_IO_stderr_;
+      stdin = (FILE *) &_IO_stdin_;
+      stdout = (FILE *) &_IO_stdout_;
+      stderr = (FILE *) &_IO_stderr_;
       _IO_list_all = &_IO_stderr_;
-      _IO_stdin->_vtable_offset = _IO_stdout->_vtable_offset =
-	_IO_stderr->_vtable_offset = stdin->_vtable_offset =
-	stdout->_vtable_offset = stderr->_vtable_offset =
+      stdin->_vtable_offset = stdout->_vtable_offset
+	= stderr->_vtable_offset =
 	((int) sizeof (struct _IO_FILE)
 	 - (int) sizeof (struct _IO_FILE_complete));
     }
diff --git a/libio/stdfiles.c b/libio/stdfiles.c
index 605e006474..9c779b47eb 100644
--- a/libio/stdfiles.c
+++ b/libio/stdfiles.c
@@ -25,11 +25,10 @@ 
    in files containing the exception.  */
 
 
-/* This file provides definitions of _IO_stdin, _IO_stdout, and _IO_stderr
-   for C code.  Compare stdstreams.cc.
-   (The difference is that here the vtable field is set to 0,
-   so the objects defined are not valid C++ objects.  On the other
-   hand, we don't need a C++ compiler to build this file.) */
+/* This file provides definitions of _IO_2_1_stdin_, _IO_2_1_stdout_,
+   and _IO_2_1_stderr_, the default values of stdin, stdout, stderr.
+   See oldstdfiles.c for glibc 2.0 legacy definitions without wide
+   character support.  */
 
 #include "libioP.h"
 
diff --git a/libio/stdio.c b/libio/stdio.c
index 1294d2842e..522de44a27 100644
--- a/libio/stdio.c
+++ b/libio/stdio.c
@@ -33,14 +33,3 @@ 
 FILE *stdin = (FILE *) &_IO_2_1_stdin_;
 FILE *stdout = (FILE *) &_IO_2_1_stdout_;
 FILE *stderr = (FILE *) &_IO_2_1_stderr_;
-
-#undef _IO_stdin
-#undef _IO_stdout
-#undef _IO_stderr
-#define AL(name) AL2 (name, _IO_##name)
-#define AL2(name, al) \
-  extern __typeof (name) al __attribute__ ((alias (#name),                    \
-                                            visibility ("hidden")))
-AL(stdin);
-AL(stdout);
-AL(stderr);