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

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Mon, Feb 03, 2014 at 11:53:25AM -0500, Carlos O'Donell wrote:
> 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.

I sent an email to the original author, but did not get any response.
I did look closely at the original test case though - that's how I
figured out the bug.  However, the test I have written now is
different from that test:

- I don't have a hard-coded set of strings; I generate a set of just
  two strings from a single character.

- I adapted the test case to plug into our testsuite

- Other than that, the test is a simple fputws, ftell in a loop and
  finally an fclose(), which is barely 10 lines of code.

I don't know what level of paranoia is appropriate for such things
since I don't have enough experience on it, so I'll just assume that
it's appropriate.  Let me know if my effort to differentiate this test
from the original one is not enough so that I will just stick to just
committing the patch and let someone else write the test from scratch.

> > +/* 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?

Nothing really, just to make it that bit faster.  In fact the minimum
number is somewhere around 1387 or something like that.  I could just
write it up as:

/* Arbitrary number large enough that the target buffer during
   conversion is not large enough.  */

since the 'small enough' just adds confusion and nothing else.

> > +  /* 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.

This is again an arbitrary character I chose because it seems to
occupy an odd number of bytes after conversion, making the condition
easy to reproduce.  I could write this as:

/* Generate input from one character.  The character is arbitrarily
   chosen since it seems to occupy an odd number of bytes after
   conversion.  */

Or do you want me to find out which character that is?  It'll be a
fair bit of work...

Siddhesh