[v2] libio: Fix stream orientation when using fread [BZ #22796]
Checks
| Context |
Check |
Description |
| redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Build passed
|
| redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
| linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
fail
|
Test failed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-arm |
fail
|
Test failed
|
Commit Message
When using fread, stream orientation is only set in the case when
less than a buffer's worth (4096 bytes) of data is requested. In this
case the __underflow function is called, and the orientation set.
In the case where more than 4096 bytes are needed, the __underflow function
is skipped, and hence the stream's orientation is left unset.
Setting the orientation to byte oriented before any buffer filling logic
ensures that the orientation is set correctly regardless of the amount of
data requested.
Signed-off-by: Samuel Zeter <samuelzeter@gmail.com>
---
libio/Makefile | 1 +
libio/fileops.c | 4 ++
libio/genops.c | 2 -
libio/tst-bz22796.c | 101 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 106 insertions(+), 2 deletions(-)
create mode 100644 libio/tst-bz22796.c
Comments
On Mär 18 2025, Samuel Zeter wrote:
> diff --git a/libio/fileops.c b/libio/fileops.c
> index a59e248142..7921fa0fb2 100644
> --- a/libio/fileops.c
> +++ b/libio/fileops.c
> @@ -1328,6 +1328,10 @@ _IO_file_xsgetn (FILE *fp, void *data, size_t n)
>
> want = n;
>
> + /* Set the stream to byte oriented */
> + if (fp->_mode == 0)
> + _IO_fwide (fp, -1);
> +
> if (fp->_IO_buf_base == NULL)
> {
> /* Maybe we already have a push back pointer. */
> diff --git a/libio/genops.c b/libio/genops.c
> index c3178d9a97..8537a20a92 100644
> --- a/libio/genops.c
> +++ b/libio/genops.c
> @@ -296,8 +296,6 @@ __underflow (FILE *fp)
> if (_IO_vtable_offset (fp) == 0 && _IO_fwide (fp, -1) != -1)
> return EOF;
>
> - if (fp->_mode == 0)
> - _IO_fwide (fp, -1);
> if (_IO_in_put_mode (fp))
> if (_IO_switch_to_get_mode (fp) == EOF)
> return EOF;
__underflow is also called in other places. Do they need to be updated?
* Samuel Zeter:
> diff --git a/libio/fileops.c b/libio/fileops.c
> index a59e248142..7921fa0fb2 100644
> --- a/libio/fileops.c
> +++ b/libio/fileops.c
> @@ -1328,6 +1328,10 @@ _IO_file_xsgetn (FILE *fp, void *data, size_t n)
>
> want = n;
>
> + /* Set the stream to byte oriented */
> + if (fp->_mode == 0)
> + _IO_fwide (fp, -1);
> +
> if (fp->_IO_buf_base == NULL)
> {
> /* Maybe we already have a push back pointer. */
Aren't there are other xsgetn methods for different stream types? Then
this change will not fix them.
I think it's better to leave orientation handling to the external
interface (so for example, __underflow (for getc_unlocked) and fread).
Thanks,
Florian
> Aren't there are other xsgetn methods for different stream types? Then
> this change will not fix them.
Thanks for the response. Unfortunately I'm new to this codebase and not
entirely sure what other methods you mean for different stream types.
I thought the issue only affected this particular call to fread.
> I think it's better to leave orientation handling to the external
> interface (so for example, __underflow (for getc_unlocked) and fread).
I'm also not sure what you mean by leaving orientation handling to the
external interface, when __underflow() itself is being called by
_IO_file_xsgetn(), at least in my test I wrote for fread. Wouldn't
that make _IO_file_xsgetn() the external interface?
I do now see __getdelim() uses __underflow() and the stream setting
I removed is probably needed there.
Thanks in advance
Sam.
* Sam Zeter:
>> Aren't there are other xsgetn methods for different stream types? Then
>> this change will not fix them.
>
> Thanks for the response. Unfortunately I'm new to this codebase and not
> entirely sure what other methods you mean for different stream types.
> I thought the issue only affected this particular call to fread.
No worries, unfortunately it's quite a maze.
>> I think it's better to leave orientation handling to the external
>> interface (so for example, __underflow (for getc_unlocked) and fread).
>
> I'm also not sure what you mean by leaving orientation handling to the
> external interface, when __underflow() itself is being called by
> _IO_file_xsgetn(), at least in my test I wrote for fread. Wouldn't
> that make _IO_file_xsgetn() the external interface?
The layering is not very strict. But there are functions that are
expected to be called by applications (such as fread, fputc, getdelim
etc.), functions are strictly internal only, and functions that can be
called from libstdc++ in GCC 2.95. Based on my understanding of the
codebase, orientation is best handled in the first category of
functions. If we do that, the internal-only functions will encounter
oriented streams automatically. The GCC 2.95 stuff does not matter for
orientation purposes because we did not have wide streams back then.
Thanks,
Florian
@@ -91,6 +91,7 @@ tests = \
tst-asprintf-null \
tst-atime \
tst-bz22415 \
+ tst-bz22796 \
tst-bz24051 \
tst-bz24153 \
tst-bz28828 \
@@ -1328,6 +1328,10 @@ _IO_file_xsgetn (FILE *fp, void *data, size_t n)
want = n;
+ /* Set the stream to byte oriented */
+ if (fp->_mode == 0)
+ _IO_fwide (fp, -1);
+
if (fp->_IO_buf_base == NULL)
{
/* Maybe we already have a push back pointer. */
@@ -296,8 +296,6 @@ __underflow (FILE *fp)
if (_IO_vtable_offset (fp) == 0 && _IO_fwide (fp, -1) != -1)
return EOF;
- if (fp->_mode == 0)
- _IO_fwide (fp, -1);
if (_IO_in_put_mode (fp))
if (_IO_switch_to_get_mode (fp) == EOF)
return EOF;
new file mode 100644
@@ -0,0 +1,101 @@
+/* Test to ensure stream orientation is set when reading large
+ buffer sizes with fread (BZ#22796).
+
+ Copyright (C) 2019-2025 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <wchar.h>
+
+#include <support/check.h>
+#include <support/temp_file.h>
+
+static int fd;
+static FILE *fp = NULL;
+static char buffer[BUFSIZ] = { 0 };
+
+static void
+setup (void)
+{
+ int ret;
+
+ fd = create_temp_file ("bz22796", NULL);
+ if (fd == -1)
+ FAIL_EXIT1 ("create_temp_file failed");
+
+ fp = fdopen (fd, "w");
+ if (fp == NULL)
+ FAIL_EXIT1 ("fopen for file bz22796 returned NULL.");
+
+ /* The stream should have no orientation set yet */
+ ret = fwide (fp, 0);
+ if (ret != 0)
+ FAIL_EXIT1 (
+ "Error: Orientation already set. fwide returned %d.\n", ret);
+}
+
+static int
+test_read_large_buffer (void)
+{
+ int ret;
+
+ setup ();
+
+ /* Read a large amount of bytes */
+ fread (buffer, 4, BUFSIZ / 4, fp);
+
+ /* Stream should now be byte orientated */
+ ret = fwide (fp, 0);
+ if (ret != -1)
+ FAIL_RET ("fread returned %d, expected -1.\n", ret);
+
+ return 0;
+}
+
+static int
+test_read_small_buffer (void)
+{
+ int ret;
+
+ setup ();
+
+ /* Read a small amount of bytes */
+ fread (buffer, 4, 4, fp);
+
+ /* Stream should now be byte orientated */
+ ret = fwide (fp, 0);
+ if (ret != -1)
+ FAIL_RET ("fread returned %d, expected -1.\n", ret);
+
+ return 0;
+}
+
+static int
+do_test (void)
+{
+ int ret = 0;
+
+ ret = test_read_small_buffer ();
+ ret += test_read_large_buffer ();
+
+ fclose (fp);
+
+ return ret;
+}
+
+#include <support/test-driver.c>