| 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=] | expand |
| 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 |
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,
> -----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
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,
> -----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.
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 --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;
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(-)