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

[Jack Haughton]

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