| Message ID | 20200731145356.7763-1-jack.haughton@broadcom.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] avutil/opt: Restore NULL input handling to set_string_video_rate() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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 [...]
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 >
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".
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".
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 [...]
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".
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,
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".
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
>
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() [...]
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
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
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".
On Thu, Aug 06, 2020 at 02:43:08PM +0200, Andreas Rheinhardt wrote: > 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? I think the handling of "not set" should be consistent between types and should follow a few properties For example Value <-> String should maintain that "not set" for Rationals, 0/0 is the natural mathetmatical way of saying not set. There is also the aspect that we may not want to allow every AVOption to be "not set" For integers a valid value has to be used for a not set form and that can then easily be bounded by the min/max so as to specify it as (not)allowed but for oters based on AVRational or floats the support for NaN is not so natural to specify. So we should probably add a AV_OPT_FLAG_ALLOW_NAN or equivalent [...] thx
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;