libio: Eliminate _IO_stdin, _IO_stdout, _IO_stderr
Commit Message
* Gabriel F. T. Gomes:
>> 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?
No, this is was a mistake on my part. 8-( I did not want to change
behavior here.
The old code in _IO_old_file_init_internal looks like this:
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))
/* The object is dynamically allocated and large enough. Initialize
the _mode element as well. */
((struct _IO_FILE_complete *) fp)->_mode = -1;
So in a new program (which defines _IO_stdin_used), we would assume the
_mode member is present. In an old program (no _IO_stdin_used), we look
at _IO_stdin etc., which will have been initialized to point to
_IO_stdin_ etc. If fp does not point to one of these objects, we write
to _mode.
So the condition matching the old code would indeed be:
&_IO_stdin_used != NULL || !_IO_legacy_file ((FILE *) fp)
New patch below.
libio is somewhat inconsistent in the way it checks for legacy file
handles. freopen uses the absence of _IO_stdin_used. _IO_old_fopen
(the old implementation of fopen) does *not* look at _IO_stdin_used and
always allocates a large struct, which includes the _mode member:
$ gdb libio/oldiofopen.os
…
(gdb) print sizeof(struct _IO_FILE_complete_plus)
$1 = 152
Compare this to:
$ gdb libio/oldstdfiles.os
…
(gdb) print sizeof(_IO_stdin_)
$1 = 80
Presumably, this makes the assignment to _mode safe in all cases where
the object is not one of the pre-allocated _IO_stdin_ (etc.) objects.
It is not safe if a shared object brings its own statically allocated
FILE object (no matter how it is named) and the shared object is loaded
into a new application (which defines _IO_stdin_used). This object will
not have a _mode member, yet, _IO_old_file_init_internal tries to assign
to it.
We know that _IO_stdin_used is unreliable (see my reply to Dmitry). The
question is whether we can find a representative set of old binaries to
make changes in this area.
Thanks,
Florian
libio: Eliminate _IO_stdin, _IO_stdout, _IO_stderr
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. (The
removed variables _IO_stdin, _IO_stdout, _IO_stderr were not exported,
of course.)
2019-02-18 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.
Comments
On Mon, Feb 18, 2019 at 12:43:32PM +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. (The
> removed variables _IO_stdin, _IO_stdout, _IO_stderr were not exported,
> of course.)
>
> 2019-02-18 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 good, thanks.
On Mon, Feb 18 2019, Florian Weimer wrote:
>
> So in a new program (which defines _IO_stdin_used), we would assume the
> _mode member is present. In an old program (no _IO_stdin_used), we look
> at _IO_stdin etc., which will have been initialized to point to
> _IO_stdin_ etc. If fp does not point to one of these objects, we write
> to _mode.
>
> ...
>
> libio is somewhat inconsistent in the way it checks for legacy file
> handles. freopen uses the absence of _IO_stdin_used. _IO_old_fopen
> (the old implementation of fopen) does *not* look at _IO_stdin_used and
> always allocates a large struct, which includes the _mode member:
>
> $ gdb libio/oldiofopen.os
> …
> (gdb) print sizeof(struct _IO_FILE_complete_plus)
> $1 = 152
>
> Compare this to:
>
> $ gdb libio/oldstdfiles.os
> …
> (gdb) print sizeof(_IO_stdin_)
> $1 = 80
>
> Presumably, this makes the assignment to _mode safe in all cases where
> the object is not one of the pre-allocated _IO_stdin_ (etc.) objects.
> It is not safe if a shared object brings its own statically allocated
> FILE object (no matter how it is named) and the shared object is loaded
> into a new application (which defines _IO_stdin_used). This object will
> not have a _mode member, yet, _IO_old_file_init_internal tries to assign
> to it.
Thanks for the detailed explanations.
> libio: Eliminate _IO_stdin, _IO_stdout, _IO_stderr
>
> 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. (The
> removed variables _IO_stdin, _IO_stdout, _IO_stderr were not exported,
> of course.)
OK.
> - 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))
OK.
This version looks good to me. Thanks!
@@ -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;
}
@@ -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
@@ -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))
@@ -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;
@@ -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 { \
@@ -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;
@@ -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;
}
@@ -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));
}
@@ -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"
@@ -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);