diff mbox series

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

Message ID 20200731145356.7763-1-jack.haughton@broadcom.com
State New
Headers show
Series [FFmpeg-devel] avutil/opt: Restore NULL input handling to set_string_video_rate()
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Jack Haughton July 31, 2020, 2:53 p.m. UTC
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.
---
 libavutil/opt.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Michael Niedermayer July 31, 2020, 7:31 p.m. UTC | #1
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

[...]
Jack Haughton Aug. 2, 2020, 7:40 p.m. UTC | #2
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.

Thanks,
Jack

On Fri, Jul 31, 2020 at 8:31 PM Michael Niedermayer <michael@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
>
Michael Niedermayer Aug. 2, 2020, 10:27 p.m. UTC | #3
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@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Jack Haughton Aug. 3, 2020, 1:07 p.m. UTC | #4
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@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@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@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer Aug. 3, 2020, 10:27 p.m. UTC | #5
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

[...]
Jack Haughton Aug. 4, 2020, 8:37 a.m. UTC | #6
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@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George Aug. 4, 2020, 1:58 p.m. UTC | #7
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,
Jack Haughton Aug. 5, 2020, 1:12 p.m. UTC | #8
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 <george@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-request@ffmpeg.org with subject "unsubscribe".
Jack Haughton Aug. 5, 2020, 1:13 p.m. UTC | #9
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.
---
 libavutil/opt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavutil/opt.c b/libavutil/opt.c
index c8413fa5e1..bcb46451e0 100644
--- a/libavutil/opt.c
+++ b/libavutil/opt.c
@@ -333,7 +333,10 @@ static int set_string_image_size(void *obj, const
AVOption *o, const char *val,

 static int set_string_video_rate(void *obj, const AVOption *o, const char
*val, AVRational *dst)
 {
-    int ret = av_parse_video_rate(dst, val);
+    int ret;
+
+    av_assert0(val);
+    ret = av_parse_video_rate(dst, val);
     if (ret < 0)
         av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as
video rate\n", val);
     return ret;
--
2.17.0.windows.1

On Wed, Aug 5, 2020 at 2:12 PM Jack Haughton <jack.haughton@broadcom.com>
wrote:

> 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 <george@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-request@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
>
Michael Niedermayer Aug. 6, 2020, 9:26 a.m. UTC | #10
On Wed, Aug 05, 2020 at 02:13:06PM +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.
> ---
>  libavutil/opt.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index c8413fa5e1..bcb46451e0 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -333,7 +333,10 @@ static int set_string_image_size(void *obj, const
> AVOption *o, const char *val,
> 
>  static int set_string_video_rate(void *obj, const AVOption *o, const char
> *val, AVRational *dst)
>  {
> -    int ret = av_parse_video_rate(dst, val);
> +    int ret;
> +
> +    av_assert0(val);
> +    ret = av_parse_video_rate(dst, val);
>      if (ret < 0)
>          av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as
> video rate\n", val);
>      return ret;

there are extra line breaks in the diff so git rejectes it:

Applying: avutil/opt: Restore NULL input handling to set_string_video_rate()
error: corrupt patch at line 10
error: could not build fake ancestor
Patch failed at 0001 avutil/opt: Restore NULL input handling to set_string_video_rate()

[...]
Andreas Rheinhardt Aug. 6, 2020, 12:43 p.m. UTC | #11
Jack Haughton:
> 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.
> ---
>  libavutil/opt.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index c8413fa5e1..9f36dc89a9 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -333,7 +333,13 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val,
>  
>  static int set_string_video_rate(void *obj, const AVOption *o, const char *val, AVRational *dst)
>  {
> -    int ret = av_parse_video_rate(dst, val);
> +    int ret;
> +
> +    if (!val) {
> +        av_log(obj, AV_LOG_ERROR, "NULL passed as video rate\n");
> +        return AVERROR(EINVAL);
> +    }
> +    ret = av_parse_video_rate(dst, val);
>      if (ret < 0)
>          av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as video rate\n", val);
>      return ret;
> 
I pondered this other caller back then [1] and thought that a NULL
default value for a framerate was illegal and that therefore the
function should fail hard in such a case to force the programmer to fix
the code. But upon rereading the doc I couldn't find any documentation
that actually said that a default value of NULL is illegal. I have
probably misunderstood the "offset must point to AVRational" comment in
opt.h (which only says how the value will be stored and says nothing
about the default value)).

There is even a usecase for allowing a NULL as default value: When one
wants to know whether the user has set a value or not. (Right now this
is not possible (at least not without a second option that says that the
other framerate option is to be used), because av_parse_video_rate()
does not allow illegal values (numerator or denumerator <= 0) for the
framerate.) So a possible usecase could be as follows: If a file
contains a framerate, but if one wants to allow the user to override the
framerate, then not using a default value is natural.

Of course in this case set_string_video_rate() should not be called at
all; instead the case should be handled in av_opt_set_default2() which
should set the value explicitly to (AVRational){ 0, 0 } in this case.

What do others think about this?

- Andreas

[1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/259250.html
Andreas Rheinhardt Aug. 6, 2020, 12:51 p.m. UTC | #12
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
Jack Haughton Aug. 13, 2020, 11:42 a.m. UTC | #13
Hi Andreas,

You're right - apologies. So in fact there was an exchange of one kind of
undefined behaviour for another.

Thanks
Jack


On Thu, Aug 6, 2020 at 1:52 PM Andreas Rheinhardt <
andreas.rheinhardt@gmail.com> wrote:

> 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavutil/opt.c b/libavutil/opt.c
index c8413fa5e1..9f36dc89a9 100644
--- a/libavutil/opt.c
+++ b/libavutil/opt.c
@@ -333,7 +333,13 @@  static int set_string_image_size(void *obj, const AVOption *o, const char *val,
 
 static int set_string_video_rate(void *obj, const AVOption *o, const char *val, AVRational *dst)
 {
-    int ret = av_parse_video_rate(dst, val);
+    int ret;
+
+    if (!val) {
+        av_log(obj, AV_LOG_ERROR, "NULL passed as video rate\n");
+        return AVERROR(EINVAL);
+    }
+    ret = av_parse_video_rate(dst, val);
     if (ret < 0)
         av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as video rate\n", val);
     return ret;