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

[Jack Haughton]

I think we're talking past each other. You're asking me to demonstrate a
current way in which NULL might be passed to this function. I don't think
it's necessary to do that in order for this patch to be a good idea.

> Is this about something iam missing or a lib user passing invalidly
> setup AVOptions ? (he should fix his code)

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.

To be clear, I do not have a specific case in mind that will currently pass
NULL to this function. The point of this patch is to stop relying on all
current and future callers to this function always being completely
bug-free, but instead to handle any error condition properly, as it is at
most of the other av_parse_video_rate callsites. Could you explain why you
would not want to do that?

Thanks
Jack

On Mon, Aug 3, 2020 at 11:27 PM Michael Niedermayer <michael at niedermayer.cc>
wrote:

> On Mon, Aug 03, 2020 at 02:07:36PM +0100, Jack Haughton wrote:
> > 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
>
> who is passing NULL in which way ?
> this is not a public function
>
> You can achieve such a NULL input by seting up an invalid AVOption
> but you dont seem to speak about that.
>
> please explain what sort of robustness you want to achieve here
> Is this about something iam missing or a lib user passing invalidly
> setup AVOptions ? (he should fix his code) or some FFmpeg internal
> robustness or what else ?
>
>
> > undefined behaviour, where previously it would have been cleanly handled
> > with an explicit error code. I'd contend that this is a bad response.
>
>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> If a bugfix only changes things apparently unrelated to the bug with no
> further explanation, that is a good sign that the bugfix is wrong.
> _______________________________________________
> 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