diff mbox series

[FFmpeg-devel,V3,1/5] libavdevice/v4l2.c: fix build warning for [-Wformat-truncation=]

Message ID 20210226083727.2042-1-yejun.guo@intel.com
State New
Headers show
Series [FFmpeg-devel,V3,1/5] libavdevice/v4l2.c: fix build warning for [-Wformat-truncation=]
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Guo, Yejun Feb. 26, 2021, 8:37 a.m. UTC
Here is the warning message:
src/libavdevice/v4l2.c: In function ‘v4l2_get_device_list’:
src/libavdevice/v4l2.c:1054:58: warning: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size 251 [-Wformat-truncation=]
         snprintf(device_name, sizeof(device_name), "/dev/%s", entry->d_name);
                                                          ^~
src/libavdevice/v4l2.c:1054:9: note: ‘snprintf’ output between 6 and 261 bytes into a destination of size 256
         snprintf(device_name, sizeof(device_name), "/dev/%s", entry->d_name);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
---
 libavdevice/v4l2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nicolas George Feb. 26, 2021, 9:27 a.m. UTC | #1
Guo, Yejun (12021-02-26):
> Here is the warning message:
> src/libavdevice/v4l2.c: In function ‘v4l2_get_device_list’:
> src/libavdevice/v4l2.c:1054:58: warning: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size 251 [-Wformat-truncation=]
>          snprintf(device_name, sizeof(device_name), "/dev/%s", entry->d_name);
>                                                           ^~
> src/libavdevice/v4l2.c:1054:9: note: ‘snprintf’ output between 6 and 261 bytes into a destination of size 256
>          snprintf(device_name, sizeof(device_name), "/dev/%s", entry->d_name);
>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
> ---
>  libavdevice/v4l2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
> index 365bacd771..cb426cf2d5 100644
> --- a/libavdevice/v4l2.c
> +++ b/libavdevice/v4l2.c
> @@ -1046,7 +1046,7 @@ static int v4l2_get_device_list(AVFormatContext *ctx, AVDeviceInfoList *device_l
>          return ret;
>      }
>      while ((entry = readdir(dir))) {
> -        char device_name[256];

> +        char device_name[sizeof(entry->d_name) + 5];

Unfortunately, that is not guaranteed to work: entry->d_name can be
declared as char d_name[0] and the space dynamically allocated at the
end of the structure.

I think a better course of action would be to acknowledge that this
warning is a waste of time and disable it.

That does not mean we should not fix the cases where the buffer can be
too small. We should, and we will find them without this warning, by
making use of common sense. This instance is not one, because even with
a billion webcams, /dev/video999999999 fits in 256 chars.

>  
>          if (!v4l2_is_v4l_dev(entry->d_name))
>              continue;

Regards,
Guo, Yejun Feb. 27, 2021, 5:52 a.m. UTC | #2
> -----Original Message-----
> From: Nicolas George <george@nsup.org>
> Sent: 2021年2月26日 17:28
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Cc: Guo, Yejun <yejun.guo@intel.com>
> Subject: Re: [FFmpeg-devel] [PATCH V3 1/5] libavdevice/v4l2.c: fix build
> warning for [-Wformat-truncation=]
> 
> Guo, Yejun (12021-02-26):
> > Here is the warning message:
> > src/libavdevice/v4l2.c: In function ‘v4l2_get_device_list’:
> > src/libavdevice/v4l2.c:1054:58: warning: ‘%s’ directive output may be
> truncated writing up to 255 bytes into a region of size 251
> [-Wformat-truncation=]
> >          snprintf(device_name, sizeof(device_name), "/dev/%s",
> entry->d_name);
> >                                                           ^~
> > src/libavdevice/v4l2.c:1054:9: note: ‘snprintf’ output between 6 and 261
> bytes into a destination of size 256
> >          snprintf(device_name, sizeof(device_name), "/dev/%s",
> entry->d_name);
> >
> >
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ~
> >
> > Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
> > ---
> >  libavdevice/v4l2.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c index
> > 365bacd771..cb426cf2d5 100644
> > --- a/libavdevice/v4l2.c
> > +++ b/libavdevice/v4l2.c
> > @@ -1046,7 +1046,7 @@ static int v4l2_get_device_list(AVFormatContext
> *ctx, AVDeviceInfoList *device_l
> >          return ret;
> >      }
> >      while ((entry = readdir(dir))) {
> > -        char device_name[256];
> 
> > +        char device_name[sizeof(entry->d_name) + 5];
> 
> Unfortunately, that is not guaranteed to work: entry->d_name can be declared
> as char d_name[0] and the space dynamically allocated at the end of the
> structure.

'man readdir' on my system shows that it is d_name[256], see below.

           struct dirent {
               ino_t          d_ino;       /* Inode number */
               off_t          d_off;       /* Not an offset; see below */
               unsigned short d_reclen;    /* Length of this record */
               unsigned char  d_type;      /* Type of file; not supported
                                              by all filesystem types */
               char           d_name[256]; /* Null-terminated filename */
           };

If there is a possibility for 'char d_name[0]', yes, it is an issue in the patch.

> 
> I think a better course of action would be to acknowledge that this warning is
> a waste of time and disable it.

agree, and maybe we can just let this build warning there.

> 
> That does not mean we should not fix the cases where the buffer can be too
> small. We should, and we will find them without this warning, by making use
> of common sense. This instance is not one, because even with a billion
> webcams, /dev/video999999999 fits in 256 chars.
> 
> >
> >          if (!v4l2_is_v4l_dev(entry->d_name))
> >              continue;
> 
> Regards,
> 
> --
>   Nicolas George
Nicolas George Feb. 28, 2021, 11:45 a.m. UTC | #3
Guo, Yejun (12021-02-27):
> 'man readdir' on my system shows that it is d_name[256], see below.

man tells you what your particular system does right now, assuming the
man page is up-to-date. It does not tell you if your system will change
tomorrow, nor what other system do.

> agree, and maybe we can just let this build warning there.

In the long run, better disable it. We have check_disable_warning in
configure, so it is probably a one line patch.

Regards,
Guo, Yejun March 1, 2021, 2:12 a.m. UTC | #4
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Nicolas
> George
> Sent: 2021年2月28日 19:45
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH V3 1/5] libavdevice/v4l2.c: fix build
> warning for [-Wformat-truncation=]
> 
> Guo, Yejun (12021-02-27):
> > 'man readdir' on my system shows that it is d_name[256], see below.
> 
> man tells you what your particular system does right now, assuming the man
> page is up-to-date. It does not tell you if your system will change tomorrow,
> nor what other system do.
> 
> > agree, and maybe we can just let this build warning there.
> 
> In the long run, better disable it. We have check_disable_warning in configure,
> so it is probably a one line patch.

thanks for the advice, we might don't disable it, because the disabling will no longer
remind the developers of this issue existing, this is another comment. thanks.
Nicolas George March 1, 2021, 11:49 a.m. UTC | #5
Guo, Yejun (12021-03-01):
> thanks for the advice, we might don't disable it, because the disabling will no longer
> remind the developers of this issue existing, this is another comment. thanks.

What "issue"? The only issue here is a useless warning.

Regards,
diff mbox series

Patch

diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
index 365bacd771..cb426cf2d5 100644
--- a/libavdevice/v4l2.c
+++ b/libavdevice/v4l2.c
@@ -1046,7 +1046,7 @@  static int v4l2_get_device_list(AVFormatContext *ctx, AVDeviceInfoList *device_l
         return ret;
     }
     while ((entry = readdir(dir))) {
-        char device_name[256];
+        char device_name[sizeof(entry->d_name) + 5];
 
         if (!v4l2_is_v4l_dev(entry->d_name))
             continue;