[FFmpeg-devel,2/2] avformat/webm_chunk: Check header filename length

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/webm_chunk.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Michael Niedermayer:
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/webm_chunk.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/webm_chunk.c b/libavformat/webm_chunk.c
> index 561ec152e7..e2fbd8be1d 100644
> --- a/libavformat/webm_chunk.c
> +++ b/libavformat/webm_chunk.c
> @@ -88,6 +88,8 @@ static int get_chunk_filename(AVFormatContext *s, int is_header, char filename[M
>  {
>      WebMChunkContext *wc = s->priv_data;
>      AVFormatContext *oc = wc->avf;
> +    int len;
> +
>      if (!filename) {
>          return AVERROR(EINVAL);
>      }
> @@ -96,7 +98,11 @@ static int get_chunk_filename(AVFormatContext *s, int is_header, char filename[M
>              av_log(oc, AV_LOG_ERROR, "No header filename provided\n");
>              return AVERROR(EINVAL);
>          }
> -        av_strlcpy(filename, wc->header_filename, MAX_FILENAME_SIZE);
> +        len = av_strlcpy(filename, wc->header_filename, MAX_FILENAME_SIZE);
> +        if (len >= MAX_FILENAME_SIZE) {
> +            av_log(oc, AV_LOG_ERROR, "header filename too long\n");
> +            return AVERROR(EINVAL);
> +        }
>      } else {
>          if (av_get_frame_filename(filename, MAX_FILENAME_SIZE,
>                                    s->url, wc->chunk_index - 1) < 0) {
> 
len has an unnecessarily broad scope. The string is intentionally
started with a lower case letter because the parameter "header" is
lower case, too, isn't it? If so, it's fine and LGTM apart from the scope.

- Andreas

On Fri, May 03, 2019 at 06:31:00AM +0000, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/webm_chunk.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/webm_chunk.c b/libavformat/webm_chunk.c
> > index 561ec152e7..e2fbd8be1d 100644
> > --- a/libavformat/webm_chunk.c
> > +++ b/libavformat/webm_chunk.c
> > @@ -88,6 +88,8 @@ static int get_chunk_filename(AVFormatContext *s, int is_header, char filename[M
> >  {
> >      WebMChunkContext *wc = s->priv_data;
> >      AVFormatContext *oc = wc->avf;
> > +    int len;
> > +
> >      if (!filename) {
> >          return AVERROR(EINVAL);
> >      }
> > @@ -96,7 +98,11 @@ static int get_chunk_filename(AVFormatContext *s, int is_header, char filename[M
> >              av_log(oc, AV_LOG_ERROR, "No header filename provided\n");
> >              return AVERROR(EINVAL);
> >          }
> > -        av_strlcpy(filename, wc->header_filename, MAX_FILENAME_SIZE);
> > +        len = av_strlcpy(filename, wc->header_filename, MAX_FILENAME_SIZE);
> > +        if (len >= MAX_FILENAME_SIZE) {
> > +            av_log(oc, AV_LOG_ERROR, "header filename too long\n");
> > +            return AVERROR(EINVAL);
> > +        }
> >      } else {
> >          if (av_get_frame_filename(filename, MAX_FILENAME_SIZE,
> >                                    s->url, wc->chunk_index - 1) < 0) {
> > 
> len has an unnecessarily broad scope. The string is intentionally
> started with a lower case letter because the parameter "header" is
> lower case, too, isn't it? If so, it's fine and LGTM apart from the scope.

Ill capitalize the error message and move the "int len" into the block

thanks

[...]

Michael Niedermayer:
> On Fri, May 03, 2019 at 06:31:00AM +0000, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavformat/webm_chunk.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/webm_chunk.c b/libavformat/webm_chunk.c
>>> index 561ec152e7..e2fbd8be1d 100644
>>> --- a/libavformat/webm_chunk.c
>>> +++ b/libavformat/webm_chunk.c
>>> @@ -88,6 +88,8 @@ static int get_chunk_filename(AVFormatContext *s, int is_header, char filename[M
>>>  {
>>>      WebMChunkContext *wc = s->priv_data;
>>>      AVFormatContext *oc = wc->avf;
>>> +    int len;
>>> +
>>>      if (!filename) {
>>>          return AVERROR(EINVAL);
>>>      }
>>> @@ -96,7 +98,11 @@ static int get_chunk_filename(AVFormatContext *s, int is_header, char filename[M
>>>              av_log(oc, AV_LOG_ERROR, "No header filename provided\n");
>>>              return AVERROR(EINVAL);
>>>          }
>>> -        av_strlcpy(filename, wc->header_filename, MAX_FILENAME_SIZE);
>>> +        len = av_strlcpy(filename, wc->header_filename, MAX_FILENAME_SIZE);
>>> +        if (len >= MAX_FILENAME_SIZE) {
>>> +            av_log(oc, AV_LOG_ERROR, "header filename too long\n");
>>> +            return AVERROR(EINVAL);
>>> +        }
>>>      } else {
>>>          if (av_get_frame_filename(filename, MAX_FILENAME_SIZE,
>>>                                    s->url, wc->chunk_index - 1) < 0) {
>>>
>> len has an unnecessarily broad scope. The string is intentionally
>> started with a lower case letter because the parameter "header" is
>> lower case, too, isn't it? If so, it's fine and LGTM apart from the scope.
> 
> Ill capitalize the error message and move the "int len" into the block
> 
> thanks
> 
Good. The patch that apparently aroused your interest in the
webm_chunk muxer was part of a patchset that was mainly about fixing
ticket #5752, a NULL pointer dereference. Would you mind taking a look
at the other patches? They are here:
https://ffmpeg.org/pipermail/ffmpeg-devel/2019-April/242903.html
https://ffmpeg.org/pipermail/ffmpeg-devel/2019-April/242902.html

- Andreas