[FFmpeg-devel] [PATCH v3 2/2] avformat: add data_size for ff_hex_to_data()

[Andreas Rheinhardt]

lance.lmwang at gmail.com:
> From: Limin Wang <lance.lmwang at gmail.com>
> 
> This prevents OOM in case of data buffer size is insufficient.

OOM?

> 
> Signed-off-by: Limin Wang <lance.lmwang at gmail.com>
> ---
>  libavformat/hls.c          | 4 +++-
>  libavformat/internal.h     | 6 ++++--
>  libavformat/rtpdec_latm.c  | 6 ++++--
>  libavformat/rtpdec_mpeg4.c | 6 ++++--
>  libavformat/utils.c        | 7 +++++--
>  5 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index 8fc6924..d7d0387 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -800,7 +800,9 @@ static int parse_playlist(HLSContext *c, const char *url,
>              if (!strcmp(info.method, "SAMPLE-AES"))
>                  key_type = KEY_SAMPLE_AES;
>              if (!av_strncasecmp(info.iv, "0x", 2)) {
> -                ff_hex_to_data(iv, info.iv + 2);
> +                ret = ff_hex_to_data(iv, sizeof(iv), info.iv + 2);
> +                if (ret < 0)
> +                    goto fail;
>                  has_iv = 1;
>              }
>              av_strlcpy(key, info.uri, sizeof(key));
> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index d57e63c..3192aca 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -423,10 +423,12 @@ char *ff_data_to_hex(char *buf, const uint8_t *src, int size, int lowercase);
>   * digits is ignored.
>   *
>   * @param data if non-null, the parsed data is written to this pointer
> + * @param data_size the data buffer size
>   * @param p the string to parse
> - * @return the number of bytes written (or to be written, if data is null)
> + * @return the number of bytes written (or to be written, if data is null),
> + *  or < 0 in case data_size is insufficient if data isn't null.
>   */
> -int ff_hex_to_data(uint8_t *data, const char *p);
> +int ff_hex_to_data(uint8_t *data, int data_size, const char *p);

This is unnecessary, as none of the callers need it: The rtpdec users
call ff_hex_to_data() twice, once to get the necessary size, once to
write the data. And the hls buffer is already big enough. I only see two
things that could be improved: Return size_t in ff_hex_to_data() as
that's the proper type (this includes checks in the callers for whether
the return type fit into the type of the extradata size)) and making the
size of the iv automatically match the needed size of (struct keyinfo).iv.

>  
>  /**
>   * Add packet to an AVFormatContext's packet_buffer list, determining its
> diff --git a/libavformat/rtpdec_latm.c b/libavformat/rtpdec_latm.c
> index 104a00a..cf1d581 100644
> --- a/libavformat/rtpdec_latm.c
> +++ b/libavformat/rtpdec_latm.c
> @@ -91,7 +91,7 @@ static int latm_parse_packet(AVFormatContext *ctx, PayloadContext *data,
>  
>  static int parse_fmtp_config(AVStream *st, const char *value)
>  {
> -    int len = ff_hex_to_data(NULL, value), i, ret = 0;
> +    int len = ff_hex_to_data(NULL, 0, value), i, ret = 0;
>      GetBitContext gb;
>      uint8_t *config;
>      int audio_mux_version, same_time_framing, num_programs, num_layers;
> @@ -100,7 +100,9 @@ static int parse_fmtp_config(AVStream *st, const char *value)
>      config = av_mallocz(len + AV_INPUT_BUFFER_PADDING_SIZE);
>      if (!config)
>          return AVERROR(ENOMEM);
> -    ff_hex_to_data(config, value);
> +    ret = ff_hex_to_data(config, len, value);
> +    if (ret < 0)
> +        return ret;
>      init_get_bits(&gb, config, len*8);
>      audio_mux_version = get_bits(&gb, 1);
>      same_time_framing = get_bits(&gb, 1);
> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
> index 34c7950..27751df 100644
> --- a/libavformat/rtpdec_mpeg4.c
> +++ b/libavformat/rtpdec_mpeg4.c
> @@ -112,11 +112,13 @@ static void close_context(PayloadContext *data)
>  static int parse_fmtp_config(AVCodecParameters *par, const char *value)
>  {
>      /* decode the hexa encoded parameter */
> -    int len = ff_hex_to_data(NULL, value), ret;
> +    int len = ff_hex_to_data(NULL, 0, value), ret;
>  
>      if ((ret = ff_alloc_extradata(par, len)) < 0)
>          return ret;
> -    ff_hex_to_data(par->extradata, value);
> +    ret = ff_hex_to_data(par->extradata, par->extradata_size, value);
> +    if (ret < 0)
> +        return ret;>      return 0;
>  }
>  
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 9228313..dfe9f4c 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -4768,7 +4768,7 @@ char *ff_data_to_hex(char *buff, const uint8_t *src, int s, int lowercase)
>      return buff;
>  }
>  
> -int ff_hex_to_data(uint8_t *data, const char *p)
> +int ff_hex_to_data(uint8_t *data, int data_size, const char *p)
>  {
>      int c, len, v;
>  
> @@ -4787,8 +4787,11 @@ int ff_hex_to_data(uint8_t *data, const char *p)
>              break;
>          v = (v << 4) | c;
>          if (v & 0x100) {
> -            if (data)
> +            if (data) {
> +                if (len >= data_size)
> +                    return AVERROR(ERANGE);
>                  data[len] = v;
> +            }
>              len++;
>              v = 1;
>          }
>