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

["Carlos O'Donell" <carlos at redhat dot com>]

On 02/03/2014 11:21 PM, Siddhesh Poyarekar wrote:
> 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.

I agree, that's sufficiently paranoid.

Now it's on record :-)

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

Looks good.

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

Allan solved that for you.

Cheers,
Carlos.