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

[Andreas Rheinhardt]

lance.lmwang at gmail.com:
> On Mon, May 10, 2021 at 09:52:48PM +0200, Andreas Rheinhardt wrote:
>> 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?
> Yes, it's invalid Out Of Memory access, not no memory available. 
> What's your suggestion?
> 

What you mean is commonly called "buffer overflow"; OOM exclusively
means "no memory available".
I already told you what I think should be done below.

>>
>>>
>>> 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;
>>>          }
>>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>