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

[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.

Thanks
Jack

On Sun, Aug 2, 2020 at 11:28 PM Michael Niedermayer <michael at niedermayer.cc>
wrote:

> On Sun, Aug 02, 2020 at 08:40:27PM +0100, Jack Haughton wrote:
> > Hello,
> >
> > Apologies for the delay in replying. This patch is not to address a
> > specific problem that currently exists, but rather to restore the
> > robustness to invalid input that previously existed in this function. The
> > motivation for the patch is that we would like to merge a500b975 into our
> > local tree to gain support for gcc 10, but there are concerns about the
> > removal of the null check. av_parse_video_rate() passes its argument
> > directly to strcmp, which would cause undefined behaviour if the argument
> > was NULL.
>
> now it makes it even less sense to me.
> av_parse_video_rate() doesnt support a NULL argument before or after your
> patch, its undefined behaviour either way.
> what you change is a private function using that public function.
>
> Now if you changed the public function to do something specific with a NULL
> argument then that would be more robust maybe but not when its done to a
> single caller which should not pass NULL there
>
> thx
>
>
>
> >
> > Thanks,
> > Jack
> >
> > On Fri, Jul 31, 2020 at 8:31 PM Michael Niedermayer
> <michael at niedermayer.cc>
> > wrote:
> >
> > > Hi
> > >
> > > On Fri, Jul 31, 2020 at 03:53:56PM +0100, Jack Haughton wrote:
> > > > Commit a500b975 removed NULL input handling from this function,
> > > > moving the check higher up the call tree in one branch. However,
> > > > there is another call to set_string_video_rate() which may pass
> > > > NULL, and future users of the function may not be clear that
> > > > a NULL check is required. This patch restores the NULL check to
> > > > avoid these problems.
> > >
> > > Does this affect something else than the seting based on defaults ?
> > > because the defaults should probably be valid values
> > > or if you disagree, iam curious where the default would be
> intentionally
> > > invalid
> > >
> > > thx
> > >
> > > [...]
> > > --
> > > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> > >
> > > Breaking DRM is a little like attempting to break through a door even
> > > though the window is wide open and the only thing in the house is a
> bunch
> > > of things you dont want and which you would get tomorrow for free
> anyway
> > >
> >
> >
> > --
> >
> > 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 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".
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> It is what and why we do it that matters, not just one of them.
> _______________________________________________
> 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