Re: [BZ #14090] Fix md5/sha512 with large block sizes

[Roland McGrath <roland at hack dot frob dot com>]

> I simplified the test and now it tests up to 10 GB (error starts with
> sizes >= 8 GB) which means an mmap of 10 GB and RSS of up to 400
> MB. 

Is this the only way to reproduce the problem?  Or can it be done without
the giant buffer by calling __md5_process_bytes many times?

I'm surprised it requires so much RSS.  AIUI normally all zero-fill pages
share the same physical page.

> But the md5 calculation is rather slow, so it takes on my fast machine
> nearly a minute.
> 
> Should we run this as part of make check - or add it to xcheck
> instead?

I don't know if we've established rules about that.
I don't mind 'make check' taking an extra minute or two, myself.
But having it swamp the memory resources on smaller machines is
not so nice.

> 	[BZ #14090]
> 	* crypt/md5test2.c: New test, based on test supplied by Serge
> 	Belyshev <belyshev@depni.sinp.msu.ru>.
> 	* crypt/Makefile (tests): Add md5test2.

IIRC the fix also touched the sha* code.  So there should be a test for
that too.  Perhaps the two tests can share some code.

I'd prefer a more descriptive name, like test-md5-10gb or test-md5-giant or
something.

> diff --git a/crypt/md5test2.c b/crypt/md5test2.c
> new file mode 100644
> index 0000000..925cc18
> --- /dev/null
> +++ b/crypt/md5test2.c
> @@ -0,0 +1,106 @@
> +/* Testcase for http://sourceware.org/bugzilla/show_bug.cgi?id=14090.  */

I think we are nowadays putting copyright headers on test cases.

> +#define CONST_2G  0x080000000
> +#define CONST_10G 0x280000000

Personally I find it more readable to write (2 << 30) and (10 << 30).
But the latter needs to use ULL or UINT64_C.

> +  const char ref [16];

No space before [] (and several more places).

> +/* test md5 in a single md5_process_bytes call.  */

Capitalize the sentence.

> +/* test md5 with two md5_process_bytes calls to trigger a
> +   different path in md5_process_block for sizes > 2 GB.  */

Here too.

> +  buf = mmap (0, CONST_10G, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> +  if (!buf)

Don't use implicit boolean coercion.  Anyway, this needs to compare against
MAP_FAILED, not zero.

This can't work on configurations where size_t is 32 bits, and probably
produces compile-time errors there without a cast.  So I'd say surround the
whole test with 'if (sizeof (void *) > 4)' or somesuch.

> +    {
> +      puts ("Could not allocate 10 GB via mmap, skipping test.\n");

puts already adds a newline.

> +  for (j = 0; j < sizeof (test_data) / sizeof (struct test_data_s); j ++)

Spurious space before ++.

> +    {
> +      result += test_single (buf, test_data [j].len, test_data [j].ref);
> +      result += test_double (buf, test_data [j].len, test_data [j].ref);
> +    }
> +
> +  return result;
> +}

I don't think "exit status of number of failures" is a very good practice.
Just make it 'if (test...) result = 1;' in the loop.

> +#define TIMEOUT 120

Add a comment about how long it takes.  If what you experience on a fast
machine is about a minute, I'd scale the timeout up a fair bit more than 2x.


Thanks,
Roland