| Message ID | 20221127173343.2577-1-alessandro.dinepi@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavc/videotoolbox: validate vt context in the decoder callback | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Just to add that this fix, once approved, should be cherry-picked to all the release branches where d7f4ad88a0df3c1339e142957bf2c40cd056b8ce has been cherry-picked. Basically, 4.4, 5.0, and 5.1. Thanks On 27 Nov 2022, 19:34 +0200, Alessandro Di Nepi <alessandro.dinepi@gmail.com>, wrote: > The commit d7f4ad88a0df3c1339e142957bf2c40cd056b8ce introduced a race > condition where the passed opaque pointer reference might be NULL, > when the decoding process starts. > This patch checks that vtctx has a value before accessing it. > > This patch fixes #10079. > > Signed-off-by: Alessandro Di Nepi <alessandro.dinepi@gmail.com> > --- > libavcodec/videotoolbox.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c > index 1b1be8ddb4..615e2b087a 100644 > --- a/libavcodec/videotoolbox.c > +++ b/libavcodec/videotoolbox.c > @@ -692,6 +692,11 @@ static void videotoolbox_decoder_callback(void *opaque, > { > VTContext *vtctx = opaque; > > + if (!vtctx) { > + av_log(NULL, AV_LOG_WARNING, "vt decoder cb: vt context is null"); > + return; > + } > + > if (vtctx->frame) { > CVPixelBufferRelease(vtctx->frame); > vtctx->frame = NULL; > -- > 2.37.1 (Apple Git-137.1) >
A gentle reminder for this patch: it's a simple fix that prevents crashing on iOS. Thanks
On Tue, Nov 29, 2022 at 11:47 AM Alessandro Di Nepi < alessandro.dinepi@gmail.com> wrote: > Just to add that this fix, once approved, should be cherry-picked to all > the release branches where d7f4ad88a0df3c1339e142957bf2c40cd056b8ce has > been cherry-picked. > Basically, 4.4, 5.0, and 5.1. > > Thanks > On 27 Nov 2022, 19:34 +0200, Alessandro Di Nepi < > alessandro.dinepi@gmail.com>, wrote: > > The commit d7f4ad88a0df3c1339e142957bf2c40cd056b8ce introduced a race > > condition where the passed opaque pointer reference might be NULL, > > when the decoding process starts. > > This patch checks that vtctx has a value before accessing it. > > > > This patch fixes #10079. > > > > Signed-off-by: Alessandro Di Nepi <alessandro.dinepi@gmail.com> > > --- > > libavcodec/videotoolbox.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c > > index 1b1be8ddb4..615e2b087a 100644 > > --- a/libavcodec/videotoolbox.c > > +++ b/libavcodec/videotoolbox.c > > @@ -692,6 +692,11 @@ static void videotoolbox_decoder_callback(void > *opaque, > > { > > VTContext *vtctx = opaque; > > > > + if (!vtctx) { > When this happens, does it continue happening, or is it transient? My main concern is log spamming. > > + av_log(NULL, AV_LOG_WARNING, "vt decoder cb: vt context is null"); > > + return; > > + } > > + > > if (vtctx->frame) { > > CVPixelBufferRelease(vtctx->frame); > > vtctx->frame = NULL; > > -- > > 2.37.1 (Apple Git-137.1) > > > _______________________________________________ > 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 4 Dec 2022, 17:01 +0200, FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>, wrote: > When this happens, does it continue happening, or is it transient? My main > concern is log spamming. Good question: this is just a transient state, so that it won't continue happening. To give you some context: when the decoding start, the value of `vtctx` is captured "too" early so that the first time the callback is called, it's still NULL. The next time it will have a proper value.
On Sun, Dec 4, 2022 at 12:51 PM Alessandro Di Nepi < alessandro.dinepi@gmail.com> wrote: > On 4 Dec 2022, 17:01 +0200, FFmpeg development discussions and patches < > ffmpeg-devel@ffmpeg.org>, wrote: > > When this happens, does it continue happening, or is it transient? My > main > > concern is log spamming. > Good question: this is just a transient state, so that it won't continue > happening. > To give you some context: when the decoding start, the value of `vtctx` is > captured "too" early so that the first time the callback is called, it's > still NULL. > The next time it will have a proper value. > If the code isn't setting a variable in time, that issue should be fixed. Otherwise the decoder will drop frames.
> On Dec 5, 2022, at 21:36, Rick Kern <kernrj@gmail.com> wrote: > > On Sun, Dec 4, 2022 at 12:51 PM Alessandro Di Nepi < > alessandro.dinepi@gmail.com> wrote: > >> On 4 Dec 2022, 17:01 +0200, FFmpeg development discussions and patches < >> ffmpeg-devel@ffmpeg.org>, wrote: >>> When this happens, does it continue happening, or is it transient? My >> main >>> concern is log spamming. >> Good question: this is just a transient state, so that it won't continue >> happening. >> To give you some context: when the decoding start, the value of `vtctx` is >> captured "too" early so that the first time the callback is called, it's >> still NULL. >> The next time it will have a proper value. >> > If the code isn't setting a variable in time, that issue should be fixed. > Otherwise the decoder will drop frames. Yes, null pointer check doesn’t looks like a resolution to a race condition. I’m not sure how the race condition happened in the first place. > > _______________________________________________ >> 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". >> > _______________________________________________ > 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".
Got you; giving some context here, and you can find all the details in the ticket #10079 (http://trac.ffmpeg.org/ticket/10079). The issue has been introduced with the commit d7f4ad88a0df3c1339e142957bf2c40cd056b8ce. This patch basically changed: • In the function `videotoolbox_start(AVCodecContext *avctx)`, ``` - decoder_cb.decompressionOutputRefCon = avctx; + decoder_cb.decompressionOutputRefCon = avctx->internal->hwaccel_priv_data; ``` • The context is retrieved in the function, `videotoolbox_decoder_callback(...)` ``` - AVCodecContext *avctx = opaque; - VTContext *vtctx = avctx->internal->hwaccel_priv_data; + VTContext *vtctx = opaque; ``` Having said that, I see that when the `videotoolbox_start` is called, • `avctx` is not NULL, • `avctx->internal->hwaccel_priv_data` is NULL The first time the `videotoolbox_decoder_callback` is called, `avctx->internal->hwaccel_priv_data` now has a value, so before d7f4ad88a `vtctx` has a value. After the change, since `avctx->internal->hwaccel_priv_data` is captured in `video toolbox_start`, is NULL and `vtctx` is also NULL. Again, this happens just the first time the callback is called; from the second time, vtctx has a proper value, and everything proceeds as expected. I'm willing to change the patch if you think there is a better way, but something needs to be done because the library simply crashes in the current state. From what I see from the original change, reverting is not an option. Looking forward to hear feedback on this. Best Regards Alessandro On 6 Dec 2022, 7:20 +0200, "zhilizhao(赵志立)" <quinkblack@foxmail.com>, wrote: > > > On Dec 5, 2022, at 21:36, Rick Kern <kernrj@gmail.com> wrote: > > > > On Sun, Dec 4, 2022 at 12:51 PM Alessandro Di Nepi < > > alessandro.dinepi@gmail.com> wrote: > > > > > On 4 Dec 2022, 17:01 +0200, FFmpeg development discussions and patches < > > > ffmpeg-devel@ffmpeg.org>, wrote: > > > > When this happens, does it continue happening, or is it transient? My > > > main > > > > concern is log spamming. > > > Good question: this is just a transient state, so that it won't continue > > > happening. > > > To give you some context: when the decoding start, the value of `vtctx` is > > > captured "too" early so that the first time the callback is called, it's > > > still NULL. > > > The next time it will have a proper value. > > > > > If the code isn't setting a variable in time, that issue should be fixed. > > Otherwise the decoder will drop frames. > > Yes, null pointer check doesn’t looks like a resolution to a race > condition. I’m not sure how the race condition happened in the first > place. >
> On Dec 7, 2022, at 00:30, Alessandro Di Nepi <alessandro.dinepi@gmail.com> wrote: > > Got you; giving some context here, and you can find all the details in the ticket #10079 (http://trac.ffmpeg.org/ticket/10079). > > The issue has been introduced with the commit d7f4ad88a0df3c1339e142957bf2c40cd056b8ce. > This patch basically changed: > > • In the function `videotoolbox_start(AVCodecContext *avctx)`, > > ``` > - decoder_cb.decompressionOutputRefCon = avctx; > + decoder_cb.decompressionOutputRefCon = avctx->internal->hwaccel_priv_data; > ``` > > • The context is retrieved in the function, `videotoolbox_decoder_callback(...)` > > ``` > - AVCodecContext *avctx = opaque; > - VTContext *vtctx = avctx->internal->hwaccel_priv_data; > + VTContext *vtctx = opaque; > ``` > > Having said that, I see that when the `videotoolbox_start` is called, > > • `avctx` is not NULL, > • `avctx->internal->hwaccel_priv_data` is NULL Did you call av_videotoolbox_default_init() or av_videotoolbox_default_init2()? I think that’s why avctx->internal->hwaccel_priv_data is NULL. That code path is broken. Does remove the call of av_videotoolbox_default_init()/av_videotoolbox_default_init2() works for you? > > The first time the `videotoolbox_decoder_callback` is called, `avctx->internal->hwaccel_priv_data` now has a value, so before d7f4ad88a `vtctx` has a value. > After the change, since `avctx->internal->hwaccel_priv_data` is captured in `video toolbox_start`, is NULL and `vtctx` is also NULL. > > Again, this happens just the first time the callback is called; from the second time, vtctx has a proper value, and everything proceeds as expected. > > I'm willing to change the patch if you think there is a better way, but something needs to be done because the library simply crashes in the current state. > From what I see from the original change, reverting is not an option. > > Looking forward to hear feedback on this. > > Best Regards > Alessandro > On 6 Dec 2022, 7:20 +0200, "zhilizhao(赵志立)" <quinkblack@foxmail.com>, wrote: >> >>> On Dec 5, 2022, at 21:36, Rick Kern <kernrj@gmail.com> wrote: >>> >>> On Sun, Dec 4, 2022 at 12:51 PM Alessandro Di Nepi < >>> alessandro.dinepi@gmail.com> wrote: >>> >>>> On 4 Dec 2022, 17:01 +0200, FFmpeg development discussions and patches < >>>> ffmpeg-devel@ffmpeg.org>, wrote: >>>>> When this happens, does it continue happening, or is it transient? My >>>> main >>>>> concern is log spamming. >>>> Good question: this is just a transient state, so that it won't continue >>>> happening. >>>> To give you some context: when the decoding start, the value of `vtctx` is >>>> captured "too" early so that the first time the callback is called, it's >>>> still NULL. >>>> The next time it will have a proper value. >>>> >>> If the code isn't setting a variable in time, that issue should be fixed. >>> Otherwise the decoder will drop frames. >> >> Yes, null pointer check doesn’t looks like a resolution to a race >> condition. I’m not sure how the race condition happened in the first >> place. >> > _______________________________________________ > 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 9 Dec 2022, 6:45 +0200, FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>, wrote: > Did you call av_videotoolbox_default_init() or av_videotoolbox_default_init2()? Actually yes, I call `av_videotoolbox_default_init(context);` > I think that’s why avctx->internal->hwaccel_priv_data is NULL. That code path is broken. > Does remove the call of av_videotoolbox_default_init()/av_videotoolbox_default_init2() works for you? Definitively yes, if I remove the call to `av_videotoolbox_default_init` the context is fine. Thanks for sorting this out. I'd like to contribute and have this fixed or documented for other people. What should be the desired behavior? Calling `av_videotoolbox_default_init` and having the context not NULL? Or shall we remove the init at all? Best Regards
Ping on this, Can I contribute somehow either to fix calling `av_videotoolbox_default_init` or at least update the docs? Best Regards, Alessandro On 11 Dec 2022, 11:57 +0200, Alessandro Di Nepi <alessandro.dinepi@gmail.com>, wrote: > On 9 Dec 2022, 6:45 +0200, FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>, wrote: > > Did you call av_videotoolbox_default_init() or av_videotoolbox_default_init2()? > Actually yes, I call `av_videotoolbox_default_init(context);` > > I think that’s why avctx->internal->hwaccel_priv_data is NULL. That code path is broken. > > Does remove the call of av_videotoolbox_default_init()/av_videotoolbox_default_init2() works for you? > Definitively yes, if I remove the call to `av_videotoolbox_default_init` the context is fine. > > Thanks for sorting this out. I'd like to contribute and have this fixed or documented for other people. > What should be the desired behavior? Calling `av_videotoolbox_default_init` and having the context not NULL? > Or shall we remove the init at all? > > Best Regards
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Alessandro Di Nepi > Sent: 2022年12月15日 22:16 > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH] lavc/videotoolbox: validate vt context in the decoder callback > > Ping on this, > Can I contribute somehow either to fix calling `av_videotoolbox_default_init` or at least update the docs? Updating the docs doesn't work. This is a public API, it must be fixed properly: don't drop a feature, do what it was supposed to do. You can give a try, but I'm afraid it's a tricky regression. > > Best Regards, > Alessandro > On 11 Dec 2022, 11:57 +0200, Alessandro Di Nepi <alessandro.dinepi@gmail.com>, wrote: > > On 9 Dec 2022, 6:45 +0200, FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>, wrote: > > > Did you call av_videotoolbox_default_init() or av_videotoolbox_default_init2()? > > Actually yes, I call `av_videotoolbox_default_init(context);` > > > I think that’s why avctx->internal->hwaccel_priv_data is NULL. That code path is broken. > > > Does remove the call of av_videotoolbox_default_init()/av_videotoolbox_default_init2() works for you? > > Definitively yes, if I remove the call to `av_videotoolbox_default_init` the context is fine. > > > > Thanks for sorting this out. I'd like to contribute and have this fixed or documented for other people. > > What should be the desired behavior? Calling `av_videotoolbox_default_init` and having the context not NULL? > > Or shall we remove the init at all? > > > > Best Regards > _______________________________________________ > 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".
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Alessandro Di Nepi > Sent: 2022年12月15日 22:16 > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH] lavc/videotoolbox: validate vt context in the decoder callback > > Ping on this, > Can I contribute somehow either to fix calling `av_videotoolbox_default_init` or at least update the docs? Hope this patch set fixed the original usecase. http://ffmpeg.org/pipermail/ffmpeg-devel/2023-January/305477.html > > Best Regards, > Alessandro > On 11 Dec 2022, 11:57 +0200, Alessandro Di Nepi <alessandro.dinepi@gmail.com>, wrote: > > On 9 Dec 2022, 6:45 +0200, FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>, wrote: > > > Did you call av_videotoolbox_default_init() or av_videotoolbox_default_init2()? > > Actually yes, I call `av_videotoolbox_default_init(context);` > > > I think that’s why avctx->internal->hwaccel_priv_data is NULL. That code path is broken. > > > Does remove the call of av_videotoolbox_default_init()/av_videotoolbox_default_init2() works for you? > > Definitively yes, if I remove the call to `av_videotoolbox_default_init` the context is fine. > > > > Thanks for sorting this out. I'd like to contribute and have this fixed or documented for other people. > > What should be the desired behavior? Calling `av_videotoolbox_default_init` and having the context not NULL? > > Or shall we remove the init at all? > > > > Best Regards > _______________________________________________ > 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 --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c index 1b1be8ddb4..615e2b087a 100644 --- a/libavcodec/videotoolbox.c +++ b/libavcodec/videotoolbox.c @@ -692,6 +692,11 @@ static void videotoolbox_decoder_callback(void *opaque, { VTContext *vtctx = opaque; + if (!vtctx) { + av_log(NULL, AV_LOG_WARNING, "vt decoder cb: vt context is null"); + return; + } + if (vtctx->frame) { CVPixelBufferRelease(vtctx->frame); vtctx->frame = NULL;
The commit d7f4ad88a0df3c1339e142957bf2c40cd056b8ce introduced a race condition where the passed opaque pointer reference might be NULL, when the decoding process starts. This patch checks that vtctx has a value before accessing it. This patch fixes #10079. Signed-off-by: Alessandro Di Nepi <alessandro.dinepi@gmail.com> --- libavcodec/videotoolbox.c | 5 +++++ 1 file changed, 5 insertions(+)