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 _______________________________________________ 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".