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

[Andreas Rheinhardt]

Jack Haughton:
> A NULL check in av_parse_video_rate() would certainly not be a bad idea. It
> wouldn't (on its own) restore the NULL input robustness to
> set_string_video_rate() though, as any error value returned from
> av_parse_video_rate() would result in val being logged using %s, which is
> the whole problem that a500b975 was aiming to solve.
> 
> av_parse_video_rate() is also called from a lot more places (most of which
> already have an explicit NULL check immediately before the call), so
> changing the behaviour of that function by adding a NULL check potentially
> has a lot more knock-on effects than simply restoring the previously
> existing robustness to set_string_video_rate(), which is also in keeping
> with the other av_parse_video_rate() callsites.
> 
> You say that the caller 'should not' pass NULL to set_string_video_rate().
> Absolutely, agreed, but the point of checks is to handle unexpected
> situations. If someone does pass NULL, as of a500b975 the response will be
> undefined behaviour, where previously it would have been cleanly handled
> with an explicit error code. I'd contend that this is a bad response.
> 
You are contradicting yourself here: As the commit message of a500b975
makes clear and as you explain in the first paragraph, a NULL value
would not have been handled cleanly before a500b975: Using NULL for %s
is undefined behaviour, too. Just wanted to mention it because you
repeat this point later in another mail:

Jack Haughton:
> Isn't it better for that condition to be handled cleanly (as it was
> before a500b975) rather than causing undefined behaviour?

- Andreas