[FFmpeg-devel] [PATCH] Add tests for functions in hash.c

[James Almer]

On 3/6/2016 4:28 PM, NagaChaitanya Vellanki wrote:
> ---
>  libavutil/Makefile       |  1 +
>  libavutil/hash.c         | 41 +++++++++++++++++++++++++++++++++++++++++
>  tests/fate/libavutil.mak |  4 ++++
>  tests/ref/fate/hash      | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 91 insertions(+)
>  create mode 100644 tests/ref/fate/hash
> 
> diff --git a/libavutil/Makefile b/libavutil/Makefile
> index 934564f..58df75a 100644
> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -186,6 +186,7 @@ TESTPROGS = adler32                                                     \
>              file                                                        \
>              fifo                                                        \
>              float_dsp                                                   \
> +            hash                                                        \
>              hmac                                                        \
>              lfg                                                         \
>              lls                                                         \
> diff --git a/libavutil/hash.c b/libavutil/hash.c
> index 7037b0d..3515e7f 100644
> --- a/libavutil/hash.c
> +++ b/libavutil/hash.c
> @@ -237,3 +237,44 @@ void av_hash_freep(AVHashContext **ctx)
>          av_freep(&(*ctx)->ctx);
>      av_freep(ctx);
>  }
> +
> +#ifdef TEST
> +// LCOV_EXCL_START
> +
> +int main(int argc, char *argv[])

I don't see arguments being used anywhere.

> +{
> +   struct AVHashContext *ctx = NULL;
> +   int i, j;
> +   for(i = 0; i < NUM_HASHES; i++) {
> +       uint8_t src[64], dst[64];

Move src outside the for statement, init it with { 0 } instead of memset and make it static
const.
And for dst you should use AV_HASH_MAX_SIZE. Or rather, a value derived from it. See below.

> +       memset(src, 0, 64);
> +       memset(dst, 0, 64);
> +       if(av_hash_alloc(&ctx, av_hash_names(i)) == 0) {

if (!av_hash_alloc(&ctx, av_hash_names(i))) {

Alternatively you could do

if(av_hash_alloc(&ctx, av_hash_names(i)) < 0)
    return 1;

> +           av_hash_init(ctx);
> +           av_hash_update(ctx, src, 64);
> +           av_hash_final_hex(ctx, dst, 64);

64 bytes (current value of AV_HASH_MAX_SIZE) is not enough to write the hex digest of
algorithms like SHA512. Notice in the ref file that the digest of SHA256, SHA384 and SHA512
are the same length, because the latter two got truncated.
You need a bigger dst buffer.

> +           printf("%s hex: %s\n", av_hash_names(i), dst);

Use av_hash_get_name(ctx) here and below to also test that function while at it.

> +
> +           memset(dst, 0, 64);
> +           av_hash_final_bin(ctx, dst, 64);

As you probably noticed in the ref file, calling a final() function twice doesn't output the
same digest with most algorithms. Ideally you'd reinit then recalculate the digest before
every final() call.

> +           printf("%s bin: ", av_hash_names(i));
> +           for(j = 0; j < av_hash_get_size(ctx); j++) {
> +               printf("%#x ", dst[j]);
> +           }
> +           printf("\n");
> +
> +           memset(dst, 0, 64);
> +           av_hash_final_b64(ctx, dst, 64);

Same as hex, AV_HASH_MAX_SIZE is not enough for base64.

> +           printf("%s b64: %s\n", av_hash_names(i), dst);
> +           av_hash_freep(&ctx);
> +       }
> +   }
> +
> +   if(av_hash_alloc(&ctx, "FOO") != AVERROR(EINVAL)) {
> +       printf("Invalid hash type 'FOO'\n");

av_hash_alloc() may return ENOMEM, so this error message could be misleading.
Also, I'm not sure about the usefulness of this check to being with.

> +   }
> +   return 0;
> +}
> +
> +// LCOV_EXCL_STOP
> +#endif