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

[Guo, Yejun]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Reimar
> D?ffinger
> Sent: 2021年2月26日 17:21
> To: FFmpeg development discussions and patches <ffmpeg-devel at 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 at 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.