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

[Nicolas George]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200804/ba450f60/attachment.sig>