[FFmpeg-devel] [PATCH] [GSOC]add a fuzz testing in libavcodec/tests

[Andreas Rheinhardt]

a397341575 at 163.com:
> From: toseven <Byone.heng at gmail.com>
> 
> ---
>  libavcodec/tests/target_avpacket_fuzzer.c | 114 ++++++++++++++++++++++
>  1 file changed, 114 insertions(+)
>  create mode 100644 libavcodec/tests/target_avpacket_fuzzer.c
> 
> diff --git a/libavcodec/tests/target_avpacket_fuzzer.c b/libavcodec/tests/target_avpacket_fuzzer.c
> new file mode 100644
> index 0000000000..22f9898210
> --- /dev/null
> +++ b/libavcodec/tests/target_avpacket_fuzzer.c
> @@ -0,0 +1,114 @@
> +/*
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <inttypes.h>
> +#include <string.h>
> +#include "libavcodec/avcodec.h"
> +#include "libavutil/error.h"
> +#include "config.h"
> +
> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
> +
> +static int setup_side_data_entry(AVPacket* avpkt)
> +{
> +    const char *data_name = NULL;
> +    int ret = 0, bytes;
> +    uint8_t *extra_data = NULL;
> +
> +
> +    /* get side_data_name string */
> +    data_name = av_packet_side_data_name(AV_PKT_DATA_NEW_EXTRADATA);
> +
> +    /* Allocate a memory bloc */
> +    bytes = strlen(data_name);
> +
> +    if(!(extra_data = (uint8_t *)av_malloc(bytes))){

Extradata is supposed to be padded with AV_INPUT_BUFFER_PADDING_BYTES
and I guess the same requirement exists for extradata in side-data
(although it doesn't seem to be explicitly stated).

> +        ret = AVERROR(ENOMEM);
> +        fprintf(stderr, "Error occurred: %s\n", av_err2str(ret));
> +        exit(1);
> +    }
> +    /* copy side_data_name to extra_data array */
> +    memcpy(extra_data, data_name, bytes);

Isn't it the point of fuzzing to use nondeterministic data?

> +
> +    /* create side data for AVPacket */
> +    ret = av_packet_add_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA,
> +                                        extra_data, bytes);
> +    if(ret < 0){
> +        fprintf(stderr,
> +                "Error occurred in av_packet_add_side_data: %s\n",
> +                av_err2str(ret));

You are leaking extra_data here.

> +    }
> +
> +    return ret;
> +}
> +
> +static int initializations(AVPacket* avpkt)
> +{
> +    av_init_packet(avpkt);

Statement before declaration.

> +    int ret;
> +    ret = setup_side_data_entry(avpkt);
> +
> +    return ret;
> +}
> +
> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) 
> +{
> +    AVPacket avpkt;
> +    memcmp(&avpkt,data,sizeof(AVPacket));

memcmp? You mean memcpy, don't you? Anyway, FFmpeg is written in C90 and
that implies that statements must follow all the definitions in a block.
> +
> +    int num;
> +    memcmp(&num,data+sizeof(AVPacket),sizeof(int));

Same here.

> +    AVPacket *avpkt_clone = NULL;
> +    int ret = 0;
> +
> +    if(initializations(&avpkt) < 0){
> +        printf("failed to initialize variables\n");
> +        return 1;
> +    }
> +    /* test av_packet_clone*/
> +    avpkt_clone = av_packet_clone(&avpkt);

At this point avpkt will either contain garbage values for size and data
or it will contain something from your fuzzer data (if you replace
memcmp by memcpy above). In any case, avpkt.buf is not set and therefore
a full copy of the data will be performed and this will usually
segfault. So what's the point of this test?

> +
> +    if(!avpkt_clone) {
> +        av_log(NULL, AV_LOG_ERROR,"av_packet_clone failed to clone AVPacket\n");
> +        return 1;
> +    }
> +    /*test av_grow_packet*/
> +    if(av_grow_packet(avpkt_clone, num) < 0){

What has been said about avpkt.data and avpkt.size applies here, too.
av_grow_packet() will e.g. fail if num is < 0, but this is no bug in
av_grow_packet. So given that a nonzero return value does not really
indicate a bug I wonder what it is supposed to mean.

Btw: You are leaking avpkt and avpkt_clone here (if you have made it so
far).

> +        av_log(NULL, AV_LOG_ERROR, "av_grow_packet failed\n");
> +        return 1;
> +    }
> +    /* test size error check in av_new_packet*/
> +    if(av_new_packet(avpkt_clone, num) == 0){

av_new_packet() treats its destination as uninitialized which means that
you are leaking what is already in the avpkt_clone here.

> +        printf( "av_new_packet failed to return error "
> +                "when \"size\" parameter is too large.\n" );

? What makes you sure that the size parameter is too large?

> +        ret = 1;
> +    }
> +    /*test size error check in av_packet_from_data*/
> +    if(av_packet_from_data(avpkt_clone, avpkt_clone->data, num) == 0){

You are leaking an AVBufferRef and an AVBuffer here. And you are abusing
the AVBuffer-API (avpkt_clone->data is supposed to be owned by the
AVBuffer that avpkt_clone->buf references; it is not yours to use it for
av_packet_from_data().

> +        printf("av_packet_from_data failed to return error "
> +                "when \"size\" parameter is too large.\n" );

Same as above.

- Andreas