[FFmpeg-devel] [PATCH] avutil/opt: Restore NULL input handling to set_string_video_rate()

[Jack Haughton]

Commit a500b975 removed NULL input handling from this function,
moving the check higher up the call tree in one branch. However,
there is another call to set_string_video_rate() which may pass
NULL, and future users of the function may not be clear that
a NULL check is required. This patch restores the NULL check to
avoid these problems.
---
 libavutil/opt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavutil/opt.c b/libavutil/opt.c
index c8413fa5e1..bcb46451e0 100644
--- a/libavutil/opt.c
+++ b/libavutil/opt.c
@@ -333,7 +333,10 @@ static int set_string_image_size(void *obj, const
AVOption *o, const char *val,

 static int set_string_video_rate(void *obj, const AVOption *o, const char
*val, AVRational *dst)
 {
-    int ret = av_parse_video_rate(dst, val);
+    int ret;
+
+    av_assert0(val);
+    ret = av_parse_video_rate(dst, val);
     if (ret < 0)
         av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as
video rate\n", val);
     return ret;
--
2.17.0.windows.1

On Wed, Aug 5, 2020 at 2:12 PM Jack Haughton <jack.haughton at broadcom.com>
wrote:

> Fair enough, I agree an assert would be better - I'll send another patch.
>
> I was not intending to advocate for defensive programming - the patch was
> done this way in order to restore the previous behaviour. Although I agree
> with you that the outcome of passing NULL to strcmp is pretty much
> certainly a segmentation fault, the documentation marks it as undefined
> behaviour, which is, well, undefined - and therefore not to be relied upon
> IMO.
>
> Thanks
> Jack
>
> On Tue, Aug 4, 2020 at 2:58 PM Nicolas George <george at nsup.org> wrote:
>
>> Jack Haughton (12020-08-04):
>> > Absolutely, he should fix his code. But let's say some code gets by code
>> > review that has a corner case whereby NULL can be passed. Isn't it
>> better
>> > for that condition to be handled cleanly (as it was before a500b975)
>> rather
>> > than causing undefined behaviour? Then the error will be reported to the
>> > user with a clear error message and can be diagnosed and fixed quickly.
>> > Currently, what happens in this case will be implementation-dependent,
>> > making it much more difficult to diagnose.
>>
>> It depends on what kind of undefined behavior we are talking about.
>> Here, it is about dereferencing NULL, which always result in a
>> segmentation fault, and if we really want to make sure, we can force it
>> with an assert.
>>
>> What you are advocating is a typical example of what is called
>> "defensive programming". When there is some invariant that the code is
>> supposed to enforce (here: a pointer that should not be NULL), defensive
>> programming involves making provisions so that the programs continues
>> even if that invariant is broken.
>>
>> The thing is: defensive programming is almost always wrong. If an
>> invariant is broken, that means there is a bug. We do not know which
>> bug, but there is a bug. And since we do not know what it is, we have to
>> assume the worst is possible: exploitable security issue, silent data
>> corruption.
>>
>> And even if the worst does not happen, defensive programming makes
>> debugging harder: it hides the bugs, let the program seems to work, or
>> it delays the manifestation of the bug, and therefore requires more work
>> to track it. Also note that people will (irrationally) be in more hurry
>> to fix a crash than to fix an error message that looks clean.
>>
>> In this kind of cases, we have to ask ourselves a series of questions:
>>
>> 1. Is it convenient to let the caller pass NULL and have a sane result?
>>    → For free(), yes.
>>    → For av_parse_video_rate(), no, so we go to the next question.
>>
>> 2. Is it easy for the caller to check the validity of the parameter?
>>    → For == NULL, yes.
>>
>> Then the correct way of dealing with it is (1) document "the parameter
>> must not be NULL", (2) make sure it crashes early if the parameter is
>> NULL.
>>
>> Regards,
>>
>> --
>>   Nicolas George
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
>
>
> --
>
> Argon Design Ltd., Registration No. 06977690, a Broadcom Inc. company
>
> Registered in England with registered office at St John's Innovation
> Centre, Cowley Road, Cambridge, CB4 0WS
>


-- 

Argon Design Ltd., Registration No. 06977690, a Broadcom Inc. company

Registered in England with registered office at St John's Innovation
Centre, Cowley Road, Cambridge, CB4 0WS