Fix crash in _IO_wfile_sync (bug 20568)

Message ID mvmlfz7nat8.fsf@suse.de
State New
Headers show
Series
  • Fix crash in _IO_wfile_sync (bug 20568)
Related show

Commit Message

Andreas Schwab May 15, 2019, 12:48 p.m.
[BZ #20568]
	* libio/wfileops.c (_IO_wfile_sync): Correct last argument to
	__codecvt_do_length.
	* libio/Makefile (tests): Add tst-wfile-sync.
	($(objpfx)tst-wfile-sync.out): Depend on $(gen-locales).
	* libio/tst-wfile-sync.c: New file.
	* libio/tst-wfile-sync.input: New file.
---
 libio/Makefile             |  4 +++-
 libio/tst-wfile-sync.c     | 35 +++++++++++++++++++++++++++++++++++
 libio/tst-wfile-sync.input |  1 +
 libio/wfileops.c           |  4 ++--
 4 files changed, 41 insertions(+), 3 deletions(-)
 create mode 100644 libio/tst-wfile-sync.c
 create mode 100644 libio/tst-wfile-sync.input

-- 
2.21.0

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Comments

Florian Weimer May 15, 2019, 2:02 p.m. | #1
* Andreas Schwab:

> 	[BZ #20568]

> 	* libio/wfileops.c (_IO_wfile_sync): Correct last argument to

> 	__codecvt_do_length.

> 	* libio/Makefile (tests): Add tst-wfile-sync.

> 	($(objpfx)tst-wfile-sync.out): Depend on $(gen-locales).

> 	* libio/tst-wfile-sync.c: New file.

> 	* libio/tst-wfile-sync.input: New file.


Would you please reference commit
18d26750dd8fd328a78cf639fd0ec2494680a2a4 in the commit message?

> diff --git a/libio/Makefile b/libio/Makefile

> index 95b91084dd..a5236c7042 100644

> --- a/libio/Makefile

> +++ b/libio/Makefile

> @@ -65,7 +65,8 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \

>  	tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \

>  	tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \

>  	tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof \

> -	tst-sprintf-ub tst-sprintf-chk-ub tst-bz24051 tst-bz24153

> +	tst-sprintf-ub tst-sprintf-chk-ub tst-bz24051 tst-bz24153 \

> +	tst-wfile-sync


Okay.

> @@ -212,6 +213,7 @@ $(objpfx)tst-ungetwc1.out: $(gen-locales)

>  $(objpfx)tst-ungetwc2.out: $(gen-locales)

>  $(objpfx)tst-widetext.out: $(gen-locales)

>  $(objpfx)tst_wprintf2.out: $(gen-locales)

> +$(objpfx)tst-wfile-sync.out: $(gen-locales)

>  endif


Okay.

>  $(objpfx)test-freopen.out: test-freopen.sh $(objpfx)test-freopen

> diff --git a/libio/tst-wfile-sync.c b/libio/tst-wfile-sync.c

> new file mode 100644

> index 0000000000..e1f621ede7

> --- /dev/null

> +++ b/libio/tst-wfile-sync.c

> @@ -0,0 +1,35 @@

> +/* Test that _IO_wfile_sync does not crash (bug 20568).

> +   Copyright (C) 2016-2019 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

> +   <http://www.gnu.org/licenses/>.  */

> +

> +#include <locale.h>

> +#include <stdio.h>

> +#include <wchar.h>

> +#include <support/check.h>

> +

> +static int

> +do_test (void)

> +{

> +  TEST_VERIFY_EXIT (setlocale (LC_ALL, "de_DE.UTF-8") != NULL);

> +  TEST_VERIFY_EXIT (fgetwc (stdin) != WEOF);

> +  /* This calls _IO_wfile_sync.  */

> +  TEST_VERIFY_EXIT (setvbuf (stdin, NULL, _IONBF, 0) == 0);

> +

> +  return 0;

> +}

> +

> +#include <support/test-driver.c>


You could perhaps add

  TEST_COMPARE (xlseek (0, 0, SEEK_CUR), 1);

, to show that the underlying descriptor was updated correctly.

> diff --git a/libio/tst-wfile-sync.input b/libio/tst-wfile-sync.input

> new file mode 100644

> index 0000000000..12d0958f7a

> --- /dev/null

> +++ b/libio/tst-wfile-sync.input

> @@ -0,0 +1 @@

> +This is a test of _IO_wfile_sync.


Woah.  Magic!  I didn't know we had this Makefile rule:

$(objpfx)%.out: %.input $(objpfx)%

> diff --git a/libio/wfileops.c b/libio/wfileops.c

> index 0367643703..42aaf5e855 100644

> --- a/libio/wfileops.c

> +++ b/libio/wfileops.c

> @@ -508,11 +508,11 @@ _IO_wfile_sync (FILE *fp)

>  	     generate the wide characters up to the current reading

>  	     position.  */

>  	  int nread;

> -

> +	  size_t wnread = fp->_wide_data->_IO_read_ptr - fp->_wide_data->_IO_read_base;

>  	  fp->_wide_data->_IO_state = fp->_wide_data->_IO_last_state;

>  	  nread = (*cv->__codecvt_do_length) (cv, &fp->_wide_data->_IO_state,

>  					      fp->_IO_read_base,

> -					      fp->_IO_read_end, delta);

> +					      fp->_IO_read_end, wnread);

>  	  fp->_IO_read_ptr = fp->_IO_read_base + nread;

>  	  delta = -(fp->_IO_read_end - fp->_IO_read_base - nread);

>  	}


Okay.

The fixed-width case is not affected because there, the seek amount can
be determined directly from the distance between _IO_read_ptr and
_IO_read_end.

Thanks,
Florian
Andreas Schwab May 15, 2019, 2:13 p.m. | #2
On Mai 15 2019, Florian Weimer <fweimer@redhat.com> wrote:

> * Andreas Schwab:

>

>> 	[BZ #20568]

>> 	* libio/wfileops.c (_IO_wfile_sync): Correct last argument to

>> 	__codecvt_do_length.

>> 	* libio/Makefile (tests): Add tst-wfile-sync.

>> 	($(objpfx)tst-wfile-sync.out): Depend on $(gen-locales).

>> 	* libio/tst-wfile-sync.c: New file.

>> 	* libio/tst-wfile-sync.input: New file.

>

> Would you please reference commit

> 18d26750dd8fd328a78cf639fd0ec2494680a2a4 in the commit message?


It's related, but not necessary.  The test will crash without that.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer May 15, 2019, 2:23 p.m. | #3
* Andreas Schwab:

> On Mai 15 2019, Florian Weimer <fweimer@redhat.com> wrote:

>

>> * Andreas Schwab:

>>

>>> 	[BZ #20568]

>>> 	* libio/wfileops.c (_IO_wfile_sync): Correct last argument to

>>> 	__codecvt_do_length.

>>> 	* libio/Makefile (tests): Add tst-wfile-sync.

>>> 	($(objpfx)tst-wfile-sync.out): Depend on $(gen-locales).

>>> 	* libio/tst-wfile-sync.c: New file.

>>> 	* libio/tst-wfile-sync.input: New file.

>>

>> Would you please reference commit

>> 18d26750dd8fd328a78cf639fd0ec2494680a2a4 in the commit message?

>

> It's related, but not necessary.  The test will crash without that.


Ahh.  It might still make sense to mention that the bug becomes more
visible after that commit.  But the bug reference probably covers that.

Thanks,
Florian

Patch

diff --git a/libio/Makefile b/libio/Makefile
index 95b91084dd..a5236c7042 100644
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -65,7 +65,8 @@  tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
 	tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \
 	tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \
 	tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof \
-	tst-sprintf-ub tst-sprintf-chk-ub tst-bz24051 tst-bz24153
+	tst-sprintf-ub tst-sprintf-chk-ub tst-bz24051 tst-bz24153 \
+	tst-wfile-sync
 
 tests-internal = tst-vtables tst-vtables-interposed tst-readline
 
@@ -212,6 +213,7 @@  $(objpfx)tst-ungetwc1.out: $(gen-locales)
 $(objpfx)tst-ungetwc2.out: $(gen-locales)
 $(objpfx)tst-widetext.out: $(gen-locales)
 $(objpfx)tst_wprintf2.out: $(gen-locales)
+$(objpfx)tst-wfile-sync.out: $(gen-locales)
 endif
 
 $(objpfx)test-freopen.out: test-freopen.sh $(objpfx)test-freopen
diff --git a/libio/tst-wfile-sync.c b/libio/tst-wfile-sync.c
new file mode 100644
index 0000000000..e1f621ede7
--- /dev/null
+++ b/libio/tst-wfile-sync.c
@@ -0,0 +1,35 @@ 
+/* Test that _IO_wfile_sync does not crash (bug 20568).
+   Copyright (C) 2016-2019 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
+   <http://www.gnu.org/licenses/>.  */
+
+#include <locale.h>
+#include <stdio.h>
+#include <wchar.h>
+#include <support/check.h>
+
+static int
+do_test (void)
+{
+  TEST_VERIFY_EXIT (setlocale (LC_ALL, "de_DE.UTF-8") != NULL);
+  TEST_VERIFY_EXIT (fgetwc (stdin) != WEOF);
+  /* This calls _IO_wfile_sync.  */
+  TEST_VERIFY_EXIT (setvbuf (stdin, NULL, _IONBF, 0) == 0);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/libio/tst-wfile-sync.input b/libio/tst-wfile-sync.input
new file mode 100644
index 0000000000..12d0958f7a
--- /dev/null
+++ b/libio/tst-wfile-sync.input
@@ -0,0 +1 @@ 
+This is a test of _IO_wfile_sync.
diff --git a/libio/wfileops.c b/libio/wfileops.c
index 0367643703..42aaf5e855 100644
--- a/libio/wfileops.c
+++ b/libio/wfileops.c
@@ -508,11 +508,11 @@  _IO_wfile_sync (FILE *fp)
 	     generate the wide characters up to the current reading
 	     position.  */
 	  int nread;
-
+	  size_t wnread = fp->_wide_data->_IO_read_ptr - fp->_wide_data->_IO_read_base;
 	  fp->_wide_data->_IO_state = fp->_wide_data->_IO_last_state;
 	  nread = (*cv->__codecvt_do_length) (cv, &fp->_wide_data->_IO_state,
 					      fp->_IO_read_base,
-					      fp->_IO_read_end, delta);
+					      fp->_IO_read_end, wnread);
 	  fp->_IO_read_ptr = fp->_IO_read_base + nread;
 	  delta = -(fp->_IO_read_end - fp->_IO_read_base - nread);
 	}