| Message ID | 20240824092842.69365-1-post@frankplowman.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavc/vvc: Fix assertion bound on qPy_{a,b} | 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 |
On Sat, Aug 24, 2024 at 5:28 PM Frank Plowman <post@frankplowman.com> wrote: > Signed-off-by: Frank Plowman <post@frankplowman.com> > --- > libavcodec/vvc/ctu.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/vvc/ctu.c b/libavcodec/vvc/ctu.c > index 139d3dded5..9ac3684ec9 100644 > --- a/libavcodec/vvc/ctu.c > +++ b/libavcodec/vvc/ctu.c > @@ -115,8 +115,8 @@ static int get_qp_y_pred(const VVCLocalContext *lc) > else > qPy_a = fc->tab.qp[LUMA][(x_cb - 1) + y_cb * min_cb_width]; > > - av_assert2(qPy_a >= -fc->ps.sps->qp_bd_offset && qPy_a < 63); > - av_assert2(qPy_b >= -fc->ps.sps->qp_bd_offset && qPy_b < 63); > + av_assert2(qPy_a >= -fc->ps.sps->qp_bd_offset && qPy_a <= 63); > + av_assert2(qPy_b >= -fc->ps.sps->qp_bd_offset && qPy_b <= 63); > Hi Frank, Thank you for the patch. Perhaps we can consider removing the assert, as other processes guarantee the range, correct?" > > return (qPy_a + qPy_b + 1) >> 1; > } > -- > 2.46.0 > > _______________________________________________ > 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 25/08/2024 09:11, Nuo Mi wrote: > On Sat, Aug 24, 2024 at 5:28 PM Frank Plowman <post@frankplowman.com> wrote: > >> Signed-off-by: Frank Plowman <post@frankplowman.com> >> --- >> libavcodec/vvc/ctu.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/libavcodec/vvc/ctu.c b/libavcodec/vvc/ctu.c >> index 139d3dded5..9ac3684ec9 100644 >> --- a/libavcodec/vvc/ctu.c >> +++ b/libavcodec/vvc/ctu.c >> @@ -115,8 +115,8 @@ static int get_qp_y_pred(const VVCLocalContext *lc) >> else >> qPy_a = fc->tab.qp[LUMA][(x_cb - 1) + y_cb * min_cb_width]; >> >> - av_assert2(qPy_a >= -fc->ps.sps->qp_bd_offset && qPy_a < 63); >> - av_assert2(qPy_b >= -fc->ps.sps->qp_bd_offset && qPy_b < 63); >> + av_assert2(qPy_a >= -fc->ps.sps->qp_bd_offset && qPy_a <= 63); >> + av_assert2(qPy_b >= -fc->ps.sps->qp_bd_offset && qPy_b <= 63); >> > Hi Frank, > Thank you for the patch. > Perhaps we can consider removing the assert, as other processes guarantee > the range, correct?" Yeah it seems that way. v2 sent.
Nuo Mi (12024-08-25): > Thank you for the patch. > Perhaps we can consider removing the assert, as other processes guarantee > the range, correct?" Uh, what?!? The point of an assert is precisely to check that the “other processes” that are supposed to guarantee something are really doing it. What happens if a bug is introduced in these “other processes” that lets invalid value fall through? With the assert, the code crashes immediately. Without the assert, it causes a memory corruption, leading to silent corruption of data or exploitable security issue. Leave the asserts. Add asserts where there are none. Regards,
On Sun, Aug 25, 2024 at 8:00 PM Nicolas George <george@nsup.org> wrote: > Nuo Mi (12024-08-25): > > Thank you for the patch. > > Perhaps we can consider removing the assert, as other processes > guarantee > > the range, correct?" > > Uh, what?!? > > The point of an assert is precisely to check that the “other processes” > that are supposed to guarantee something are really doing it. > > What happens if a bug is introduced in these “other processes” that lets > invalid value fall through? > > With the assert, the code crashes immediately. > > Without the assert, it causes a memory corruption, leading to silent > corruption of data or exploitable security issue. > > Leave the asserts. Add asserts where there are none. > Hi Nicolas, Thank you for the feedback. assert0 will cause a released program to crash, and we have a task to remove assert0 in the VVC decoder. However, you're right that assert2 is intended for debugging purposes. If you prefer, we can keep the original version. > > 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". >
Nuo Mi (12024-08-25): > Thank you for the feedback. > assert0 will cause a released program to crash, and we have a task to > remove assert0 in the VVC decoder. Then something needs to be fixed. Removing the assert is not a fix. It might be worse than a fix: turning a crash into a silent corruption of data or exploitable security issue. (Reminder: in terms of badness, crash < silent corruption < security.) > However, you're right that assert2 is intended for debugging purposes. av_assert2() is no more and no less for debugging than av_assert0(), the only difference is that av_assert2() is meant for cases where the assertion is expensive in a speed-critical part of the code and disabled for normal users. Developers should always use --assert-level=2. > If you prefer, we can keep the original version. Of course not. You need to fix the bug. I do not know the technicals of this instance, but the first version of this patch looks like it could be the proper fix fix. The second version is not a fix at all, no need to know the technicals to know it. Regards,
On Sun, Aug 25, 2024 at 10:22 PM Nicolas George <george@nsup.org> wrote: > Nuo Mi (12024-08-25): > > Thank you for the feedback. > > assert0 will cause a released program to crash, and we have a task to > > remove assert0 in the VVC decoder. > > Then something needs to be fixed. Removing the assert is not a fix. It > might be worse than a fix: turning a crash into a silent corruption of > data or exploitable security issue. > > (Reminder: in terms of badness, crash < silent corruption < security.) > > > However, you're right that assert2 is intended for debugging purposes. > > av_assert2() is no more and no less for debugging than av_assert0(), the > only difference is that av_assert2() is meant for cases where the > assertion is expensive in a speed-critical part of the code and disabled > for normal users. > > Developers should always use --assert-level=2. > > > If you prefer, we can keep the original version. > > Of course not. You need to fix the bug. I do not know the technicals of > this instance, but the first version of this patch looks like it could > be the proper fix fix. The second version is not a fix at all, no need > to know the technicals to know it. > Yes, I mean the v1. sorry for misleading you. > > 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". >
Nuo Mi (12024-08-25):
> Yes, I mean the v1. sorry for misleading you.
Ah, ok. Then… maybe.
Maybe v1 is the proper fix. Maybe the proper fix is to find where qPy_a
or qPy_b are set to 63 if they can only be <63 and fix that part of the
code. Only somebody who knows the codec can say which is the proper fix,
but it is highly unlikely any other change is.
Regards,
On Sun, Aug 25, 2024 at 10:37 PM Nicolas George <george@nsup.org> wrote: > Nuo Mi (12024-08-25): > > Yes, I mean the v1. sorry for misleading you. > > Ah, ok. Then… maybe. > > Maybe v1 is the proper fix. Maybe the proper fix is to find where qPy_a > or qPy_b are set to 63 if they can only be <63 and fix that part of the > code. Only somebody who knows the codec can say which is the proper fix, > but it is highly unlikely any other change is. > Hi Nicolas, From the spec QpY = ( ( qPY_PRED + CuQpDeltaVal + 64 + 2 * QpBdOffset ) % ( 64 + QpBdOffset ) ) - QpBdOffset So qPY_A and qPY_B <= 63. Not < 63. Frank's patch will fix the assert. qPY_A and qPY_B calculations have no obvious issues. We read qPY_A and qPY_B from a table. We add an assertion here to ensure the table hasn't been polluted. Thank you > 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". >
diff --git a/libavcodec/vvc/ctu.c b/libavcodec/vvc/ctu.c index 139d3dded5..9ac3684ec9 100644 --- a/libavcodec/vvc/ctu.c +++ b/libavcodec/vvc/ctu.c @@ -115,8 +115,8 @@ static int get_qp_y_pred(const VVCLocalContext *lc) else qPy_a = fc->tab.qp[LUMA][(x_cb - 1) + y_cb * min_cb_width]; - av_assert2(qPy_a >= -fc->ps.sps->qp_bd_offset && qPy_a < 63); - av_assert2(qPy_b >= -fc->ps.sps->qp_bd_offset && qPy_b < 63); + av_assert2(qPy_a >= -fc->ps.sps->qp_bd_offset && qPy_a <= 63); + av_assert2(qPy_b >= -fc->ps.sps->qp_bd_offset && qPy_b <= 63); return (qPy_a + qPy_b + 1) >> 1; }
Signed-off-by: Frank Plowman <post@frankplowman.com> --- libavcodec/vvc/ctu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)