Re: [PATCH][BZ 15362] Fix fwrite() reading beyond end of buffer in error path

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Sat, Apr 20, 2013 at 01:12:18PM -0500, Eric Biggers wrote:
> I must say that running the test suite was not particularly easy for a newcomer.
> There were over a hundred test failures for no apparent reason regardless of
> whether my patch was applied.  Finally, based on the wiki (`make check' really
> should warn you about this itself...), I configured with '--prefix=/usr', which

Yes this is a known problem.  I'm sure there's a bug report for it
somewhere.

> fixed most of the failing tests.  However, some tests still failed even without
> my patch applied (using the master branch at commit
> e913141d5f4d4eed4b65f55b0077aeb1c8234e25):
> 
>   make[2]: *** [/home/e/src/glibc/build-master2/math/test-ldouble.out] Error 1
>   make[2]: *** [/home/e/src/glibc/build-master2/math/test-ildoubl.out] Error 1
>   make[1]: *** [math/tests] Error 2

Do you have an amd processor?  I think I had seen math test failures
on my amd fx4100 a while ago and had made a mental note to look into
it.  Like all mental notes, this one also got swapped out I guess.  In
any case, you can ignore these for your change.

>   make[2]: [/home/e/src/glibc/build-master2/posix/annexc.out] Error 1 (ignored)
>   make[2]: *** [/home/e/src/glibc/build-master2/rt/tst-cpuclock2.out] Error 1

The rt tests are unstable, so this one is known to fail sporadically.

> If there are tests that are expected to fail in some situations, is there any
> way that the test suite itself could warn of that?
> 
> I was also surprised that there seemed to be no place where a clear summary of
> the test suite results gets logged, other than by redirecting the standard error
> of 'make check -k' and grepping for the word "Error" to get past all the
> compiler warnings that also get sent to stderr.  Is there a better way to run
> the tests?

Nope, that's the best I know.  There's plenty of scope for
improvements to the testsuite.

> The fixed patch follows, although note there are still other comments in the
> same source code file that do not follow this style guideline.

They should be fixed too, but not in this patch.

> diff --git a/ChangeLog b/ChangeLog
> index 90d6d47..e978ee2 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,14 @@
> +2013-04-20  Eric Biggers  <ebiggers3@gmail.com>
> +
> +	[BZ #15362]
> +	* libio/fileops.c:  Revert problematic fixes for [BZ #11741]
> +	* libio/iofwrite.c:  Likewise.
> +	* libio/iofwrite_u.c:  Likewise.
> +	* libio/iopadn.c:  Likewise.
> +	* libio/iowpadn.c:  Likewise.
> +	* stdio-common/vfprintf.c:  Fix [BZ #11741] properly by checking whether
> +	_IO_padn() returned the full count written.
> +
>  2013-04-16  Roland McGrath  <roland@hack.frob.com>
>  

The change looks fine to me now.  As I had mentioned before, I'll wait
for a review from another maintainer and check it in if there's no
objection.

Thanks,
Siddhesh