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.haugh...@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 <geo...@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@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-requ...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".