[FFmpeg-devel,1/2] avformat/webm_chunk: Fix argument length of get_chunk_filename()

Submitted by Michael Niedermayer on May 2, 2019, 6:49 p.m.

Details

Message ID 20190502184936.8818-1-michael@niedermayer.cc
State Accepted
Commit 1a74b04737f08e2e11a02ada280407889f6cadb1
Headers show

Commit Message

Michael Niedermayer May 2, 2019, 6:49 p.m.
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/webm_chunk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andreas Rheinhardt May 3, 2019, 6:11 a.m.
Michael Niedermayer:
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/webm_chunk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/webm_chunk.c b/libavformat/webm_chunk.c
> index 2c99753b5b..561ec152e7 100644
> --- a/libavformat/webm_chunk.c
> +++ b/libavformat/webm_chunk.c
> @@ -84,7 +84,7 @@ static int chunk_mux_init(AVFormatContext *s)
>      return 0;
>  }
>  
> -static int get_chunk_filename(AVFormatContext *s, int is_header, char *filename)
> +static int get_chunk_filename(AVFormatContext *s, int is_header, char filename[MAX_FILENAME_SIZE])
>  {
>      WebMChunkContext *wc = s->priv_data;
>      AVFormatContext *oc = wc->avf;
> 
1. This is not a fix, merely a cosmetic clarification. After all, this
change does not allow the compiler to infer that every pointer
corresponding to the filename argument will point to an array of at
least MAX_FILENAME_SIZE elements. (C99 added a static keyword for this.)

2. You could just as well remove the check for whether filename is
NULL as the new form makes it clear that this function should not be
called with a NULL pointer as filename. (Of course, compilers can (and
do) already optimize the check away by looking at both calls of this
function.)

- Andreas
Michael Niedermayer May 3, 2019, 4:03 p.m.
On Fri, May 03, 2019 at 06:11:00AM +0000, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/webm_chunk.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/webm_chunk.c b/libavformat/webm_chunk.c
> > index 2c99753b5b..561ec152e7 100644
> > --- a/libavformat/webm_chunk.c
> > +++ b/libavformat/webm_chunk.c
> > @@ -84,7 +84,7 @@ static int chunk_mux_init(AVFormatContext *s)
> >      return 0;
> >  }
> >  
> > -static int get_chunk_filename(AVFormatContext *s, int is_header, char *filename)
> > +static int get_chunk_filename(AVFormatContext *s, int is_header, char filename[MAX_FILENAME_SIZE])
> >  {
> >      WebMChunkContext *wc = s->priv_data;
> >      AVFormatContext *oc = wc->avf;
> > 
> 1. This is not a fix, merely a cosmetic clarification. After all, this
> change does not allow the compiler to infer that every pointer
> corresponding to the filename argument will point to an array of at
> least MAX_FILENAME_SIZE elements. (C99 added a static keyword for this.)

static analyzers can use such hints to detect violations
but the real intent here was that the human developer would see
from just looking at the argument that it has a implied size.
And that way to avoid a mistake

[...]
Michael Niedermayer May 24, 2019, 10:23 a.m.
On Fri, May 03, 2019 at 06:11:00AM +0000, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/webm_chunk.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/webm_chunk.c b/libavformat/webm_chunk.c
> > index 2c99753b5b..561ec152e7 100644
> > --- a/libavformat/webm_chunk.c
> > +++ b/libavformat/webm_chunk.c
> > @@ -84,7 +84,7 @@ static int chunk_mux_init(AVFormatContext *s)
> >      return 0;
> >  }
> >  
> > -static int get_chunk_filename(AVFormatContext *s, int is_header, char *filename)
> > +static int get_chunk_filename(AVFormatContext *s, int is_header, char filename[MAX_FILENAME_SIZE])
> >  {
> >      WebMChunkContext *wc = s->priv_data;
> >      AVFormatContext *oc = wc->avf;
> > 
> 1. This is not a fix, merely a cosmetic clarification. After all, this
> change does not allow the compiler to infer that every pointer
> corresponding to the filename argument will point to an array of at
> least MAX_FILENAME_SIZE elements. (C99 added a static keyword for this.)
> 

> 2. You could just as well remove the check for whether filename is
> NULL as the new form makes it clear that this function should not be
> called with a NULL pointer as filename. (Of course, compilers can (and
> do) already optimize the check away by looking at both calls of this
> function.)

possible, but removing the check isnt truly related to this change

thx

[...]
Michael Niedermayer May 24, 2019, 10:24 a.m.
On Fri, May 03, 2019 at 06:03:02PM +0200, Michael Niedermayer wrote:
> On Fri, May 03, 2019 at 06:11:00AM +0000, Andreas Rheinhardt wrote:
> > Michael Niedermayer:
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavformat/webm_chunk.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/libavformat/webm_chunk.c b/libavformat/webm_chunk.c
> > > index 2c99753b5b..561ec152e7 100644
> > > --- a/libavformat/webm_chunk.c
> > > +++ b/libavformat/webm_chunk.c
> > > @@ -84,7 +84,7 @@ static int chunk_mux_init(AVFormatContext *s)
> > >      return 0;
> > >  }
> > >  
> > > -static int get_chunk_filename(AVFormatContext *s, int is_header, char *filename)
> > > +static int get_chunk_filename(AVFormatContext *s, int is_header, char filename[MAX_FILENAME_SIZE])
> > >  {
> > >      WebMChunkContext *wc = s->priv_data;
> > >      AVFormatContext *oc = wc->avf;
> > > 
> > 1. This is not a fix, merely a cosmetic clarification. After all, this
> > change does not allow the compiler to infer that every pointer
> > corresponding to the filename argument will point to an array of at
> > least MAX_FILENAME_SIZE elements. (C99 added a static keyword for this.)
> 
> static analyzers can use such hints to detect violations
> but the real intent here was that the human developer would see
> from just looking at the argument that it has a implied size.
> And that way to avoid a mistake

will apply with a improved commit message

thanks

[...]

Patch hide | download patch | download mbox

diff --git a/libavformat/webm_chunk.c b/libavformat/webm_chunk.c
index 2c99753b5b..561ec152e7 100644
--- a/libavformat/webm_chunk.c
+++ b/libavformat/webm_chunk.c
@@ -84,7 +84,7 @@  static int chunk_mux_init(AVFormatContext *s)
     return 0;
 }
 
-static int get_chunk_filename(AVFormatContext *s, int is_header, char *filename)
+static int get_chunk_filename(AVFormatContext *s, int is_header, char filename[MAX_FILENAME_SIZE])
 {
     WebMChunkContext *wc = s->priv_data;
     AVFormatContext *oc = wc->avf;