[PATCH] Fix wide stdio

[PATCH] Fix wide stdio

[Jakub Jelinek]

Hi!

On
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <wchar.h>
#include <error.h>

int
main (int argc, char **argv)
{
  char name[] = "/tmp/wprintf.out.XXXXXX";
  int fd;
  FILE *fp;
  fd = mkstemp (name);
  fp = fdopen (dup (fd), "a");
  if (fp == NULL)
    error (EXIT_FAILURE, errno, "fdopen(,\"a\")");
  setvbuf (fp, NULL, _IONBF, 0);
  fwprintf (fp, L"hello.\n");
  fclose (fp);
  return EXIT_SUCCESS;
}
testcase from Arjan we attempt to munmap fp->_shortbuf (which is fortunately
not page aligned and thus the munmap just fails).
The problem seems to be that we use just one _IO_USER_BUF bit, for two
different buffers (fp->_IO_buf_base and fp->_wide_data->_IO_buf_base).
If wide orientation used just one of these buffers, we could just clear
the narrow buffers in _IO_fwide when switching to wide orientation, but
when wide oriented, we actually use both buffers at the same time,
and they aren't necessarily both user buffers or both non-user buffers.
So, IMHO we just have to use a different bit for the
fp->_wide_data->_IO_buf_base allocation status (whether we are supposed
to free it or not).
Additionally, as _IO_new_file_close_it unconditionally overwrites
fp->_flags, we need to free not just the wide buffer for wide oriented
streams, but also the narrow one.

2006-11-24  Jakub Jelinek  <jakub@redhat.com>

	* libio/libio.h (_IO_FLAGS2_USER_WBUF): Define.
	* libio/wgenops.c (_IO_wsetb, _IO_wdefault_finish): Test and set
	_IO_FLAGS2_USER_WBUF bit in _flags2 instead of _IO_USER_BUF bit
	in _flags.
	* libio/wstrops.c (_IO_wstr_overflow, enlarge_userbuf,
	_IO_wstr_finish): Likewise.
	* libio/wmemstream.c (open_wmemstream): Likewise.
	* libio/fileops.c (_IO_new_file_close_it): Call _IO_set[bgp]
	even for wide streams.

--- libc/libio/libio.h.jj	2006-11-24 12:51:29.000000000 +0100
+++ libc/libio/libio.h	2006-11-24 13:30:23.000000000 +0100
@@ -142,6 +142,7 @@
 #ifdef _LIBC
 # define _IO_FLAGS2_FORTIFY 4
 #endif
+#define _IO_FLAGS2_USER_WBUF 8
 
 /* These are "formatting flags" matching the iostream fmtflags enum values. */
 #define _IO_SKIPWS 01
--- libc/libio/wgenops.c.jj	2006-11-24 12:51:29.000000000 +0100
+++ libc/libio/wgenops.c	2006-11-24 13:30:23.000000000 +0100
@@ -115,14 +115,14 @@ _IO_wsetb (f, b, eb, a)
      wchar_t *eb;
      int a;
 {
-  if (f->_wide_data->_IO_buf_base && !(f->_flags & _IO_USER_BUF))
+  if (f->_wide_data->_IO_buf_base && !(f->_flags2 & _IO_FLAGS2_USER_WBUF))
     FREE_BUF (f->_wide_data->_IO_buf_base, _IO_wblen (f) * sizeof (wchar_t));
   f->_wide_data->_IO_buf_base = b;
   f->_wide_data->_IO_buf_end = eb;
   if (a)
-    f->_flags &= ~_IO_USER_BUF;
+    f->_flags2 &= ~_IO_FLAGS2_USER_WBUF;
   else
-    f->_flags |= _IO_USER_BUF;
+    f->_flags2 |= _IO_FLAGS2_USER_WBUF;
 }
 INTDEF(_IO_wsetb)
 
@@ -198,7 +198,7 @@ _IO_wdefault_finish (fp, dummy)
      int dummy;
 {
   struct _IO_marker *mark;
-  if (fp->_wide_data->_IO_buf_base && !(fp->_flags & _IO_USER_BUF))
+  if (fp->_wide_data->_IO_buf_base && !(fp->_flags2 & _IO_FLAGS2_USER_WBUF))
     {
       FREE_BUF (fp->_wide_data->_IO_buf_base,
 		_IO_wblen (fp) * sizeof (wchar_t));
--- libc/libio/wstrops.c.jj	2006-11-24 12:51:29.000000000 +0100
+++ libc/libio/wstrops.c	2006-11-24 13:30:23.000000000 +0100
@@ -88,7 +88,7 @@ _IO_wstr_overflow (fp, c)
   pos = fp->_wide_data->_IO_write_ptr - fp->_wide_data->_IO_write_base;
   if (pos >= (_IO_size_t) (_IO_wblen (fp) + flush_only))
     {
-      if (fp->_flags & _IO_USER_BUF) /* not allowed to enlarge */
+      if (fp->_flags2 & _IO_FLAGS2_USER_WBUF) /* not allowed to enlarge */
 	return WEOF;
       else
 	{
@@ -182,7 +182,7 @@ enlarge_userbuf (_IO_FILE *fp, _IO_off64
   _IO_ssize_t oldend = wd->_IO_write_end - wd->_IO_write_base;
 
   /* Try to enlarge the buffer.  */
-  if (fp->_flags & _IO_USER_BUF)
+  if (fp->_flags2 & _IO_FLAGS2_USER_WBUF)
     /* User-provided buffer.  */
     return 1;
 
@@ -335,7 +335,7 @@ _IO_wstr_finish (fp, dummy)
      _IO_FILE *fp;
      int dummy;
 {
-  if (fp->_wide_data->_IO_buf_base && !(fp->_flags & _IO_USER_BUF))
+  if (fp->_wide_data->_IO_buf_base && !(fp->_flags2 & _IO_FLAGS2_USER_WBUF))
     (((_IO_strfile *) fp)->_s._free_buffer) (fp->_wide_data->_IO_buf_base);
   fp->_wide_data->_IO_buf_base = NULL;
 
--- libc/libio/wmemstream.c.jj	2006-11-24 12:51:29.000000000 +0100
+++ libc/libio/wmemstream.c	2006-11-24 13:30:23.000000000 +0100
@@ -92,7 +92,7 @@ open_wmemstream (bufloc, sizeloc)
   _IO_fwide (&new_f->fp._sf._sbf._f, 1);
   _IO_wstr_init_static (&new_f->fp._sf._sbf._f, buf,
 			_IO_BUFSIZ / sizeof (wchar_t), buf);
-  new_f->fp._sf._sbf._f._flags &= ~_IO_USER_BUF;
+  new_f->fp._sf._sbf._f._flags2 &= ~_IO_FLAGS2_USER_WBUF;
   new_f->fp._sf._s._allocate_buffer = (_IO_alloc_type) malloc;
   new_f->fp._sf._s._free_buffer = (_IO_free_type) free;
 
--- libc/libio/fileops.c.jj	2006-11-24 12:51:29.000000000 +0100
+++ libc/libio/fileops.c	2006-11-24 13:34:37.000000000 +0100
@@ -174,14 +174,8 @@ _IO_new_file_close_it (fp)
   close_status = _IO_SYSCLOSE (fp);
 
   /* Free buffer. */
-  if (fp->_mode <= 0)
-    {
-      INTUSE(_IO_setb) (fp, NULL, NULL, 0);
-      _IO_setg (fp, NULL, NULL, NULL);
-      _IO_setp (fp, NULL, NULL);
-    }
 #if defined _LIBC || defined _GLIBCPP_USE_WCHAR_T
-  else
+  if (fp->_mode > 0)
     {
       if (_IO_have_wbackup (fp))
 	INTUSE(_IO_free_wbackup_area) (fp);
@@ -190,6 +184,9 @@ _IO_new_file_close_it (fp)
       _IO_wsetp (fp, NULL, NULL);
     }
 #endif
+  INTUSE(_IO_setb) (fp, NULL, NULL, 0);
+  _IO_setg (fp, NULL, NULL, NULL);
+  _IO_setp (fp, NULL, NULL);
 
   INTUSE(_IO_un_link) ((struct _IO_FILE_plus *) fp);
   fp->_flags = _IO_MAGIC|CLOSED_FILEBUF_FLAGS;

	Jakub

Re: [PATCH] Fix wide stdio

[Ulrich Drepper]

Jakub Jelinek wrote:
> testcase from Arjan we attempt to munmap fp->_shortbuf (which is fortunately
> not page aligned and thus the munmap just fails).

No.  We have this bug in the upstream BZ for some time and even had one 
of the IBM people try to provide a patch.  It's not that easy.  The 
current code is screwed up big time when it comes to wide streams and 
setvbuf.  It's not such a trivial change.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

Re: [PATCH] Fix wide stdio

[Jakub Jelinek]

On Fri, Nov 24, 2006 at 07:17:22AM -0800, Ulrich Drepper wrote:
> Jakub Jelinek wrote:
> >testcase from Arjan we attempt to munmap fp->_shortbuf (which is 
> >fortunately
> >not page aligned and thus the munmap just fails).
> 
> No.  We have this bug in the upstream BZ for some time and even had one 
> of the IBM people try to provide a patch.  It's not that easy.  The 
> current code is screwed up big time when it comes to wide streams and 
> setvbuf.  It's not such a trivial change.

I'd like to understand what's still screwed up big time after the patch
I posted.  For _IONBF I don't see what is broken, the _IO_UNBUFFERED
flag is set correctly in that case and _IO_buf_base is _shortbuf
while _wide_data->_IO_buf_base is _wide_data->_shortbuf (and _end
similarly).
In other cases things IMHO work just fine too, the only question is
what is the user passed buffer supposed to be used for in wide IO,
we need 2 kinds of buffers, one char buffer, one wchar_t buffer.
Currently the user buffer is used as the char buffer and uses a
default sized allocated buffer for the other cases.  I don't think
that violates POSIX, but if e.g. the user buffer is intentionally small,
then it might not be 100% what the user would expect.
Alternatives include (when not unbuffered, which is correctly handled
with the _shortbuf buffers):
- _IO_wide would allocate a wide (non-user) buffer with
  size in some correlation to the size of the narrow buffer
  (be it (_IO_buf_end - _IO_buf_base) * sizeof (wchar_t), or
  (_IO_buf_end - _IO_buf_base) / average_length_of_mb_char * sizeof (wchar_t),
  (_IO_buf_end - _IO_buf_base) or something similar
- use user provided buffer as the wide buffer (after aligning it if needed)
  and allocate a new narrow buffer, either with a size in some correlation
  to the wide buffer size or using default size
- split the user provided buffer into two regions, use one region for the
  wide buffer and the other region for the narrow buffer
It is certainly not obvious what the right behavior is and I think neither
of the 4 alternatives (including the current one) violates the standard.

I looked at BZ#2337.
http://sources.redhat.com/bugzilla/show_bug.cgi?id=2337#c4
lists several options to fix the bug as well:

Solution 1.) in there is basically the middle alternative above with default
size narrow buffer, but in itself isn't the right fix, as e.g. the wide
buffer can still be a user buffer, while narrow buffer default allocated
and so sharing the same _IO_USER_BUF bit for it is bad.

Solution 2a.), i.e. replace fp->_flags = _IO_MAGIC|CLOSED_FILEBUF_FLAGS;
in _IO_new_file_close_it with
fp->_flags = _IO_MAGIC|CLOSED_FILEBUF_FLAGS | (fp->_flags & _IO_USER_BUF);
is certainly possible, but only if we guarantee that at any time either
both narrow and wide buffers are malloced, or are alloced by user (resp.
_shortbuf).

Solution 2b.) which has a patch provided sounds very wrong, that just
means we will leak the narrow buffer.  wdefault_finish makes sense for
wstrops (where we don't use two kinds of buffers, just the wide one).

The patch I posted had two parts, one was the addition of
_IO_FLAGS2_USER_WBUF flag and using it wherever the wide buf was meant,
the other was doing _IO_setb (fp, NULL, NULL, 0) even for wide oriented
streams in _IO_new_file_close_it.  I believe the latter is a nicer
alternative to 2a.) above and then either we need the former (USER_WBUF
flag), or we need to guarantee both buffers have the same allocation
status (this would essentially require the "user user suplied buffer for
both narrow and wide buffers").

	Jakub

Re: [PATCH] Fix wide stdio

[Ulrich Drepper]

Jakub Jelinek wrote:
> I'd like to understand what's still screwed up big time after the patch
> I posted.

Any buffer provided must be associated with the wide char buffer for 
wide streams, not the underlying temporary buffer.  This requires, if 
setvbuf is called before the decision is made, to move the buffer from 
being used in the output buffer to being used as the wide char buffer. 
Along with handling alignment issues etc this is a terribly complex change.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

Re: [PATCH] Fix wide stdio

[Jakub Jelinek]

On Mon, Nov 27, 2006 at 07:13:34AM -0800, Ulrich Drepper wrote:
> Jakub Jelinek wrote:
> >I'd like to understand what's still screwed up big time after the patch
> >I posted.
> 
> Any buffer provided must be associated with the wide char buffer for 
> wide streams, not the underlying temporary buffer.  This requires, if 
> setvbuf is called before the decision is made, to move the buffer from 
> being used in the output buffer to being used as the wide char buffer. 
> Along with handling alignment issues etc this is a terribly complex change.

This updated patch should handle that.  It doesn't really swap the buffers
(which is not necessary IMHO, no app should care about that), just sizes
the wide buffer the same as how big the setvbuf buffer is.  If setvbuf has
not been called (and the narrow buffer is therefore libio allocated,
not user allocated), it preserves the old behavior where the wide buffer
has been sizeof (wchar_t) times bigger than the narrow buffer.

It also fixes a bug where
f = fopen ("/dev/tty", "w"); setvbuf (f, buf, _IOFBF, len); fwprintf (f, L"something");
would not honor the full buffered request, _IO_wfile_doallocate used
to override it to line buffered.  Also, one unneeded stat has been removed.

2006-12-08  Jakub Jelinek  <jakub@redhat.com>

	* libio/genops.c (__uflow): Fix a typo.
	* libio/wfiledoalloc.c (_IO_wfile_doallocate): Don't stat
	nor set _IO_LINE_BUF bit here.  Size the wide buffer based on
	the narrow buffer size.

2006-11-24  Jakub Jelinek  <jakub@redhat.com>

	* libio/libio.h (_IO_FLAGS2_USER_WBUF): Define.
	* libio/wgenops.c (_IO_wsetb, _IO_wdefault_finish): Test and set
	_IO_FLAGS2_USER_WBUF bit in _flags2 instead of _IO_USER_BUF bit
	in _flags.
	* libio/wstrops.c (_IO_wstr_overflow, enlarge_userbuf,
	_IO_wstr_finish): Likewise.
	* libio/wmemstream.c (open_wmemstream): Likewise.
	* libio/fileops.c (_IO_new_file_close_it): Call _IO_set[bgp]
	even for wide streams.

--- libc/libio/fileops.c	19 Jan 2006 00:38:35 -0000	1.110
+++ libc/libio/fileops.c	5 Dec 2006 15:39:22 -0000
@@ -174,14 +174,8 @@ _IO_new_file_close_it (fp)
   close_status = _IO_SYSCLOSE (fp);
 
   /* Free buffer. */
-  if (fp->_mode <= 0)
-    {
-      INTUSE(_IO_setb) (fp, NULL, NULL, 0);
-      _IO_setg (fp, NULL, NULL, NULL);
-      _IO_setp (fp, NULL, NULL);
-    }
 #if defined _LIBC || defined _GLIBCPP_USE_WCHAR_T
-  else
+  if (fp->_mode > 0)
     {
       if (_IO_have_wbackup (fp))
 	INTUSE(_IO_free_wbackup_area) (fp);
@@ -190,6 +184,9 @@ _IO_new_file_close_it (fp)
       _IO_wsetp (fp, NULL, NULL);
     }
 #endif
+  INTUSE(_IO_setb) (fp, NULL, NULL, 0);
+  _IO_setg (fp, NULL, NULL, NULL);
+  _IO_setp (fp, NULL, NULL);
 
   INTUSE(_IO_un_link) ((struct _IO_FILE_plus *) fp);
   fp->_flags = _IO_MAGIC|CLOSED_FILEBUF_FLAGS;
--- libc/libio/genops.c	1 Mar 2006 05:32:48 -0000	1.70
+++ libc/libio/genops.c	5 Dec 2006 15:39:22 -0000
@@ -367,7 +367,7 @@ __uflow (fp)
 #endif
 
   if (fp->_mode == 0)
-    _IO_fwide (fp, -11);
+    _IO_fwide (fp, -1);
   if (_IO_in_put_mode (fp))
     if (INTUSE(_IO_switch_to_get_mode) (fp) == EOF)
       return EOF;
--- libc/libio/libio.h	14 Jan 2006 12:09:46 -0000	1.64
+++ libc/libio/libio.h	5 Dec 2006 15:39:22 -0000
@@ -142,6 +142,7 @@
 #ifdef _LIBC
 # define _IO_FLAGS2_FORTIFY 4
 #endif
+#define _IO_FLAGS2_USER_WBUF 8
 
 /* These are "formatting flags" matching the iostream fmtflags enum values. */
 #define _IO_SKIPWS 01
--- libc/libio/wfiledoalloc.c	6 Jul 2002 06:35:54 -0000	1.6
+++ libc/libio/wfiledoalloc.c	5 Dec 2006 15:39:22 -0000
@@ -1,4 +1,5 @@
-/* Copyright (C) 1993, 1997, 1999, 2000, 2002 Free Software Foundation, Inc.
+/* Copyright (C) 1993, 1997, 1999, 2000, 2002, 2006
+   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
@@ -85,35 +86,20 @@ _IO_wfile_doallocate (fp)
      _IO_FILE *fp;
 {
   _IO_size_t size;
-  int couldbetty;
   wchar_t *p;
-  struct _G_stat64 st;
 
   /* Allocate room for the external buffer.  */
   if (fp->_IO_buf_base == NULL)
     INTUSE(_IO_file_doallocate) (fp);
 
-  if (fp->_fileno < 0 || _IO_SYSSTAT (fp, &st) < 0)
-    {
-      couldbetty = 0;
-      size = _IO_BUFSIZ;
-#if 0
-      /* do not try to optimise fseek() */
-      fp->_flags |= __SNPT;
-#endif
-    }
-  else
-    {
-      couldbetty = S_ISCHR (st.st_mode);
-#if _IO_HAVE_ST_BLKSIZE
-      size = st.st_blksize <= 0 ? _IO_BUFSIZ : st.st_blksize;
-#else
-      size = _IO_BUFSIZ;
-#endif
-    }
+  /* If narrow buffer is user allocated (set by setvbuf etc.),
+     use that size as the size of the wide buffer, when it is
+     allocated by _IO_file_doallocate, multiply that by size
+     of the wide character.  */
+  size = fp->_IO_buf_end - fp->_IO_buf_base;
+  if ((fp->_flags & _IO_USER_BUF))
+    size = (size + sizeof (wchar_t) - 1) / sizeof (wchar_t);
   ALLOC_WBUF (p, size * sizeof (wchar_t), EOF);
   INTUSE(_IO_wsetb) (fp, p, p + size, 1);
-  if (couldbetty && isatty (fp->_fileno))
-    fp->_flags |= _IO_LINE_BUF;
   return 1;
 }
--- libc/libio/wgenops.c	12 Feb 2006 21:20:57 -0000	1.14
+++ libc/libio/wgenops.c	5 Dec 2006 15:39:22 -0000
@@ -115,14 +115,14 @@ _IO_wsetb (f, b, eb, a)
      wchar_t *eb;
      int a;
 {
-  if (f->_wide_data->_IO_buf_base && !(f->_flags & _IO_USER_BUF))
+  if (f->_wide_data->_IO_buf_base && !(f->_flags2 & _IO_FLAGS2_USER_WBUF))
     FREE_BUF (f->_wide_data->_IO_buf_base, _IO_wblen (f) * sizeof (wchar_t));
   f->_wide_data->_IO_buf_base = b;
   f->_wide_data->_IO_buf_end = eb;
   if (a)
-    f->_flags &= ~_IO_USER_BUF;
+    f->_flags2 &= ~_IO_FLAGS2_USER_WBUF;
   else
-    f->_flags |= _IO_USER_BUF;
+    f->_flags2 |= _IO_FLAGS2_USER_WBUF;
 }
 INTDEF(_IO_wsetb)
 
@@ -198,7 +198,7 @@ _IO_wdefault_finish (fp, dummy)
      int dummy;
 {
   struct _IO_marker *mark;
-  if (fp->_wide_data->_IO_buf_base && !(fp->_flags & _IO_USER_BUF))
+  if (fp->_wide_data->_IO_buf_base && !(fp->_flags2 & _IO_FLAGS2_USER_WBUF))
     {
       FREE_BUF (fp->_wide_data->_IO_buf_base,
 		_IO_wblen (fp) * sizeof (wchar_t));
--- libc/libio/wmemstream.c	30 Sep 2006 00:06:33 -0000	1.3
+++ libc/libio/wmemstream.c	5 Dec 2006 15:39:22 -0000
@@ -92,7 +92,7 @@ open_wmemstream (bufloc, sizeloc)
   _IO_fwide (&new_f->fp._sf._sbf._f, 1);
   _IO_wstr_init_static (&new_f->fp._sf._sbf._f, buf,
 			_IO_BUFSIZ / sizeof (wchar_t), buf);
-  new_f->fp._sf._sbf._f._flags &= ~_IO_USER_BUF;
+  new_f->fp._sf._sbf._f._flags2 &= ~_IO_FLAGS2_USER_WBUF;
   new_f->fp._sf._s._allocate_buffer = (_IO_alloc_type) malloc;
   new_f->fp._sf._s._free_buffer = (_IO_free_type) free;
 
--- libc/libio/wstrops.c	14 Aug 2006 22:14:53 -0000	1.11
+++ libc/libio/wstrops.c	5 Dec 2006 15:39:22 -0000
@@ -88,7 +88,7 @@ _IO_wstr_overflow (fp, c)
   pos = fp->_wide_data->_IO_write_ptr - fp->_wide_data->_IO_write_base;
   if (pos >= (_IO_size_t) (_IO_wblen (fp) + flush_only))
     {
-      if (fp->_flags & _IO_USER_BUF) /* not allowed to enlarge */
+      if (fp->_flags2 & _IO_FLAGS2_USER_WBUF) /* not allowed to enlarge */
 	return WEOF;
       else
 	{
@@ -182,7 +182,7 @@ enlarge_userbuf (_IO_FILE *fp, _IO_off64
   _IO_ssize_t oldend = wd->_IO_write_end - wd->_IO_write_base;
 
   /* Try to enlarge the buffer.  */
-  if (fp->_flags & _IO_USER_BUF)
+  if (fp->_flags2 & _IO_FLAGS2_USER_WBUF)
     /* User-provided buffer.  */
     return 1;
 
@@ -335,7 +335,7 @@ _IO_wstr_finish (fp, dummy)
      _IO_FILE *fp;
      int dummy;
 {
-  if (fp->_wide_data->_IO_buf_base && !(fp->_flags & _IO_USER_BUF))
+  if (fp->_wide_data->_IO_buf_base && !(fp->_flags2 & _IO_FLAGS2_USER_WBUF))
     (((_IO_strfile *) fp)->_s._free_buffer) (fp->_wide_data->_IO_buf_base);
   fp->_wide_data->_IO_buf_base = NULL;
 


	Jakub

Re: [PATCH] Fix wide stdio

[Ulrich Drepper]

I don't particularly like the patch since I think the wide buffer should 
be controlled by the user.  But

a) it's a lot of work and I don't foresee having time for something as
    unimportant as this

and

b) nobody really cares about this (the effects should be opaque)


If and when the opportunity arrives to make the correct fix it can be 
made on top of the new code.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖