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

[Reimar Döffinger]

> 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

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