This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][BZ #16398] Fix infinite loop in ftell when writing wide char data


On 02/03/2014 04:53 AM, Siddhesh Poyarekar wrote:
> On Fri, Jan 31, 2014 at 11:26:06AM -0800, Roland McGrath wrote:
>> Most test cases are pretty small such that either copyright doesn't apply
>> (if really tiny) or it's simple enough to rewrite the case from scratch
>> once you roughly grok the bug.  
> 
> OK, I just rewrote the test case.  Tested on x86_64 and ppc64.  OK to
> commit?

In the future, to be entirely kosher, I think someone other than you should
have rewritten the test case given a description of the bug provided by you.

I'm hoping that you didn't look too closely at the original test case and
that the original copyright owner of said test case doesn't actually care.

However, in the future please find someone else to rewrite the test case.
If you think I'm being too paranoid please say so, otherwise I'll continue
to be at this level of paranoia.

> Siddhesh
> 
> 	[BZ #16398]
> 	* libio/wfileops.c (_IO_wfile_seekoff): Break out form
> 	conversion when destination buffer does not have enough space.
> 	* libio/tst-ftell-partial-wide.c: New test case.
> 	* libio/Makefile (tests): Add tst-ftell-partial-wide.

Looks OK to me, but two comments need updating with some more information.
 
> commit 6be430aa57633d9a4c383e0bb72806eb81243d79
> Author: Siddhesh Poyarekar <siddhesh@redhat.com>
> Date:   Mon Feb 3 15:20:44 2014 +0530
> 
>     Fix infinite loop in ftell when writing wide char data (BZ #16398)
>     
>     ftell tries to avoid flushing the buffer when it is in write mode by
>     converting the wide char data and placing it into the binary buffer.
>     If the output buffer space is full and there is data to write, the
>     code reverts to flushing the buffer.  This breaks when there is space
>     in the buffer but it is not enough to convert the next character in
>     the wide data buffer, due to which __codecvt_do_out returns a
>     __codecvt_partial status.  In this case, ftell keeps running in an
>     infinite loop.
>     
>     The fix here is to detect the __codecvt_partial status in addition to
>     checking if the buffer is full.  I have also added a test case
>     (written by Arjun Shankar) that demonstrates the infinite loop.
> 
> diff --git a/libio/Makefile b/libio/Makefile
> index 05432f4..747a779 100644
> --- a/libio/Makefile
> +++ b/libio/Makefile
> @@ -60,7 +60,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
>  	tst-wmemstream1 tst-wmemstream2 \
>  	bug-memstream1 bug-wmemstream1 \
>  	tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \
> -	tst-fwrite-error
> +	tst-fwrite-error tst-ftell-partial-wide
>  ifeq (yes,$(build-shared))
>  # Add test-fopenloc only if shared library is enabled since it depends on
>  # shared localedata objects.
> diff --git a/libio/tst-ftell-partial-wide.c b/libio/tst-ftell-partial-wide.c
> new file mode 100644
> index 0000000..0c4dede
> --- /dev/null
> +++ b/libio/tst-ftell-partial-wide.c
> @@ -0,0 +1,104 @@
> +/* Verify that ftell does not go into an infinite loop when a conversion fails
> +   due to insufficient space in the buffer.
> +   Copyright (C) 2014 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 <wchar.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <locale.h>
> +#include <errno.h>
> +#include <unistd.h>
> +
> +/* Defined in test-skeleton.c.  */
> +static int create_temp_file (const char *base, char **filename);
> +
> +/* Large enough that the target buffer during conversion is not large
> +   enough.  I found this by just tinkering with the numbers till I found a
> +   small enough number.  */

What's wrong with making this number larger? Why does it have to be small enough?

> +#define STRING_SIZE (1400)
> +#define NSTRINGS (2)
> +
> +int
> +do_test (void)
> +{
> +  FILE *fp = NULL;
> +  wchar_t *inputs[NSTRINGS] = {NULL};
> +  int ret = 1;
> +
> +  if (setlocale (LC_ALL, "en_US.UTF-8") == NULL)
> +    {
> +      printf ("Cannot set en_US.UTF-8 locale.\n");
> +      goto out;
> +    }
> +
> +
> +  /* Generate input from one character.  */
> +  wchar_t seed = L'ã';

Please identify the character explicitly in a comment including
source language and UTF number and why you use this particular
cahracter.

> +  for (int i = 0; i < NSTRINGS; i++)
> +    {
> +      inputs[i] = malloc (STRING_SIZE * sizeof (wchar_t));
> +      if (inputs[i] == NULL)
> +	{
> +	  printf ("Failed to allocate memory for inputs: %m\n");
> +	  goto out;
> +	}
> +      wmemset (inputs[i], seed, STRING_SIZE - 1);
> +      inputs[i][STRING_SIZE - 1] = L'\0';
> +    }
> +
> +  char *filename;
> +  int fd = create_temp_file ("tst-fseek-wide-partial.out", &filename);
> +
> +  if (fd == -1)
> +    {
> +      printf ("create_temp_file: %m\n");
> +      goto out;
> +    }
> +
> +  fp = fdopen (fd, "w+");
> +  if (fp == NULL)
> +    {
> +      printf ("fopen: %m\n");
> +      close (fd);
> +      goto out;
> +    }
> +
> +  for (int i = 0; i < NSTRINGS; i++)
> +    {
> +      printf ("offset: %ld\n", ftell (fp));
> +      if (fputws (inputs[i], fp) == -1)
> +        {
> +          perror ("fputws");
> +	  goto out;
> +        }
> +    }
> +  ret = 0;
> +
> +out:
> +  if (fp != NULL)
> +    fclose (fp);
> +  for (int i = 0; i < NSTRINGS; i++)
> +    free (inputs[i]);
> +
> +  return ret;
> +}
> +
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> diff --git a/libio/wfileops.c b/libio/wfileops.c
> index 87d3cdc..877fc1f 100644
> --- a/libio/wfileops.c
> +++ b/libio/wfileops.c
> @@ -715,7 +715,7 @@ _IO_wfile_seekoff (fp, offset, dir, mode)
>  		       - fp->_wide_data->_IO_write_base) / clen;
>  	  else
>  	    {
> -	      enum __codecvt_result status;
> +	      enum __codecvt_result status = __codecvt_ok;
>  	      delta = (fp->_wide_data->_IO_write_ptr
>  		       - fp->_wide_data->_IO_write_base);
>  	      const wchar_t *write_base = fp->_wide_data->_IO_write_base;
> @@ -728,9 +728,12 @@ _IO_wfile_seekoff (fp, offset, dir, mode)
>  		 flush buffers for every ftell.  */
>  	      do
>  		{
> -		  /* Ugh, no point trying to avoid the flush.  Just do it
> -		     and go back to how it was with the read mode.  */
> -		  if (delta > 0 && new_write_ptr == fp->_IO_buf_end)
> +		  /* There is not enough space in the buffer to do the entire
> +		     conversion, so there is no point trying to avoid the
> +		     buffer flush.  Just do it and go back to how it was with
> +		     the read mode.  */
> +		  if (status == __codecvt_partial
> +		      || (delta > 0 && new_write_ptr == fp->_IO_buf_end))
>  		    {
>  		      if (_IO_switch_to_wget_mode (fp))
>  			return WEOF;
> 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]