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

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(-)

On 2/24/2021 10:38 PM, Guo, Yejun wrote:
> 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..e11d10d20e 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[512];

Is there a portable path max constant that would be better for cases 
like this, rather than just picking another arbitrary buffer size?


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

> On 25 Feb 2021, at 18:52, Chad Fraleigh <chadf@triularity.org> wrote:
> 
> On 2/24/2021 10:38 PM, Guo, Yejun wrote:
>>  libavdevice/v4l2.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
>> index 365bacd771..e11d10d20e 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[512];
> 
> Is there a portable path max constant that would be better for cases like this, rather than just picking another arbitrary buffer size?

There is PATH_MAX/MAX_PATH, however the problems are:
1) They are not necessarily a strict limit on path length, so no guarantee all file names fit
2) Even if there is such a guarantee, that only applies to VALID file names, however the files here are user input ans not necessarily valid, so using those defines does not fix things anyway

Speaking generally I have doubts that these patches actually IMPROVE security and don’t make it actually worse.
On the plus side I see:
- they fix truncations in those specific cases
On the minus side I see:
- file names can clearly be longer, so there is still a truncation issue even after these patches, truncation just happens elsewhere and at larger lengths
- the warning is removed, so now there is nothing that reminds developers of this issue existing
- it relies on “magic constants” that can easily become outdated and re-introduce the issue again at any time
- there is as far as I can tell no evidence that any of these truncations cause actual issues, or if so in which circumstances, so there is no evidence of benefit. However as with all changes there is a risk of introducing new bugs
- size of on-stack variables are increased which may decrease effectiveness of stack protection

As a result personally I am mostly worried about these patches if they are applied without a deep security review of each and ensuring the issues are understood and thoroughly fixed.
I before suggested using av_asprintf, which does at least permanently solve the truncation issue without magic constants and eliminates on-stack arrays, however I should also add that it might create a allocation failure issue, which then opens up the whole “how to handle allocation failure without introducing a even more tricky security issue”.
So I am afraid that these issues will be annoyingly costly to fix, and I am not sure how big the desire is to do so...

Best regards,
Reimar

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Reimar
> D?ffinger
> Sent: 2021年2月26日 17:21
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH V2 1/7] libavdevice/v4l2.c: fix build
> warning for [-Wformat-truncation=]
> 
> 
> > On 25 Feb 2021, at 18:52, Chad Fraleigh <chadf@triularity.org> wrote:
> >
> > On 2/24/2021 10:38 PM, Guo, Yejun wrote:
> >>  libavdevice/v4l2.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
> >> index 365bacd771..e11d10d20e 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[512];
> >
> > Is there a portable path max constant that would be better for cases like this,
> rather than just picking another arbitrary buffer size?
> 
> There is PATH_MAX/MAX_PATH, however the problems are:
> 1) They are not necessarily a strict limit on path length, so no guarantee all file
> names fit
> 2) Even if there is such a guarantee, that only applies to VALID file names,
> however the files here are user input ans not necessarily valid, so using those
> defines does not fix things anyway
> 
> Speaking generally I have doubts that these patches actually IMPROVE security
> and don’t make it actually worse.
> On the plus side I see:
> - they fix truncations in those specific cases
> On the minus side I see:
> - file names can clearly be longer, so there is still a truncation issue even after
> these patches, truncation just happens elsewhere and at larger lengths
> - the warning is removed, so now there is nothing that reminds developers of
> this issue existing
> - it relies on “magic constants” that can easily become outdated and
> re-introduce the issue again at any time
> - there is as far as I can tell no evidence that any of these truncations cause
> actual issues, or if so in which circumstances, so there is no evidence of benefit.
> However as with all changes there is a risk of introducing new bugs
> - size of on-stack variables are increased which may decrease effectiveness of
> stack protection

For the code in this function, max length of file name is fixed, see the code below.
Anyway, it still increases the on-stack variable size which might have potential security
issue.

        snprintf(device_name, sizeof(device_name), "/dev/%s", entry->d_name);

'man readdir' shows:
           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 */
           };

> 
> As a result personally I am mostly worried about these patches if they are
> applied without a deep security review of each and ensuring the issues are
> understood and thoroughly fixed.
> I before suggested using av_asprintf, which does at least permanently solve the
> truncation issue without magic constants and eliminates on-stack arrays,
> however I should also add that it might create a allocation failure issue, which
> then opens up the whole “how to handle allocation failure without
> introducing a even more tricky security issue”.
> So I am afraid that these issues will be annoyingly costly to fix, and I am not
> sure how big the desire is to do so...

with such concern, maybe we can just let this build warning there.

> On 27 Feb 2021, at 06:37, Guo, Yejun <yejun.guo@intel.com> wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Reimar
>> D?ffinger
>>> 
> 
> For the code in this function, max length of file name is fixed, see the code below.
> Anyway, it still increases the on-stack variable size which might have potential security
> issue.
> 
>        snprintf(device_name, sizeof(device_name), "/dev/%s", entry->d_name);
> 
> 'man readdir' shows:
>           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 */
>           };

The size is not standardised.
E.g. the OSX manpage has:
     struct dirent { /* when _DARWIN_FEATURE_64_BIT_INODE is defined */
             ino_t      d_fileno;     /* file number of entry */
             __uint64_t d_seekoff;    /* seek offset (optional, used by servers)
 */
             __uint16_t d_reclen;     /* length of this record */
             __uint16_t d_namlen;     /* length of string in d_name */
             __uint8_t  d_type;       /* file type, see below */
             char    d_name[1024];    /* name must be no longer than this */
     };