diff mbox series

libio: Attempt wide backup free only for non-legacy code

Message ID 20240903211402.2266742-1-siddhesh@sourceware.org
State Committed
Commit ae4d44b1d501421ad9a3af95279b8f4d1546f1ce
Headers
Series libio: Attempt wide backup free only for non-legacy code |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
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 success Test passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed

Commit Message

Siddhesh Poyarekar Sept. 3, 2024, 9:14 p.m. UTC 
  _wide_data and _mode are not available in legacy code, so do not attempt
to free the wide backup buffer in legacy code.

Resolves: BZ #32137 and BZ #27821

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---

Tested on x86_64 and i686 for any further regressions, and on i686 by
running libio/tst-stderr-compat under valgrind.

I could not think of a straightforward test case for the testsuite since
the crash due to freeing _wide_data->_IO_save_base depends on data after
_IO_FILE.  I'm relying on, instead this problem to be caught in testing
on old architectures, like it was with BZ #32137.  Maybe we also need,
e.g. a ppc32 trybot for pre-commit CI.

 libio/genops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Florian Weimer Sept. 4, 2024, 6:49 a.m. UTC | #1 
* Siddhesh Poyarekar:

> _wide_data and _mode are not available in legacy code, so do not attempt
> to free the wide backup buffer in legacy code.
>
> Resolves: BZ #32137 and BZ #27821
>
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> ---
>
> Tested on x86_64 and i686 for any further regressions, and on i686 by
> running libio/tst-stderr-compat under valgrind.
>
> I could not think of a straightforward test case for the testsuite since
> the crash due to freeing _wide_data->_IO_save_base depends on data after
> _IO_FILE.  I'm relying on, instead this problem to be caught in testing
> on old architectures, like it was with BZ #32137.  Maybe we also need,
> e.g. a ppc32 trybot for pre-commit CI.
>
>  libio/genops.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libio/genops.c b/libio/genops.c
> index 35d8b30710..6f20d49669 100644
> --- a/libio/genops.c
> +++ b/libio/genops.c
> @@ -819,7 +819,7 @@ _IO_unbuffer_all (void)
>        /* Free up the backup area if it was ever allocated.  */
>        if (_IO_have_backup (fp))
>  	_IO_free_backup_area (fp);
> -      if (fp->_mode > 0 && _IO_have_wbackup (fp))
> +      if (!legacy && fp->_mode > 0 && _IO_have_wbackup (fp))
>  	_IO_free_wbackup_area (fp);
>  
>        if (! (fp->_flags & _IO_UNBUFFERED)

This looks correct as far as it goes.  GCC could reorder the legacy
check and the fp->_mode load, but the existing code already has this
problem.

I think we should commit this and have Jeevitha P. report back if it
fixes powerpc (32-bit).

Reviewed-by: Florian Weimer <fweimer@redhat.com>

Thanks,
Florian
diff mbox series

Patch

diff --git a/libio/genops.c b/libio/genops.c
index 35d8b30710..6f20d49669 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -819,7 +819,7 @@  _IO_unbuffer_all (void)
       /* Free up the backup area if it was ever allocated.  */
       if (_IO_have_backup (fp))
 	_IO_free_backup_area (fp);
-      if (fp->_mode > 0 && _IO_have_wbackup (fp))
+      if (!legacy && fp->_mode > 0 && _IO_have_wbackup (fp))
 	_IO_free_wbackup_area (fp);
 
       if (! (fp->_flags & _IO_UNBUFFERED)