This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH][BZ #16398] Fix infinite loop in ftell when writing wide char data
- From: Siddhesh Poyarekar <siddhesh at redhat dot com>
- To: "Carlos O'Donell" <carlos at redhat dot com>
- Cc: Roland McGrath <roland at hack dot frob dot com>, Rich Felker <dalias at aerifal dot cx>, libc-alpha at sourceware dot org
- Date: Tue, 4 Feb 2014 09:51:50 +0530
- Subject: Re: [PATCH][BZ #16398] Fix infinite loop in ftell when writing wide char data
- Authentication-results: sourceware.org; auth=none
- References: <20140131051923 dot GL2149 at spoyarek dot pnq dot redhat dot com> <20140131190337 dot GX24286 at brightrain dot aerifal dot cx> <20140131192607 dot 05D5474430 at topped-with-meat dot com> <20140203095340 dot GE5209 at spoyarek dot pnq dot redhat dot com> <52EFC985 dot 7070408 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