diff mbox series

[FFmpeg-devel,2/4] avcodec/ccaption_dec: check the length of packet and return used length

Message ID 1652280498-10408-2-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/4] remove sccenc dependency on subtitles | expand

Checks

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

Commit Message

Lance Wang May 11, 2022, 2:48 p.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavcodec/ccaption_dec.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Paul B Mahol May 11, 2022, 7:47 p.m. UTC | #1 
why?
Lance Wang May 11, 2022, 11:38 p.m. UTC | #2 
On Wed, May 11, 2022 at 09:47:52PM +0200, Paul B Mahol wrote:
> why?

assuming the len is 1, the following code will access the next 3
array anymore, I think it's better to check the i with len -2.

for (i = 0; i < len; i += 3) {
to 
for (i = 0; i < len - 2; i += 3) {

for the return, I think it's correct to return the used length,
if it's error, have return already. right?
Paul B Mahol May 12, 2022, 6:25 a.m. UTC | #3 
On Thu, May 12, 2022 at 1:39 AM <lance.lmwang@gmail.com> wrote:

> On Wed, May 11, 2022 at 09:47:52PM +0200, Paul B Mahol wrote:
> > why?
>
> assuming the len is 1, the following code will access the next 3
> array anymore, I think it's better to check the i with len -2.
>
> for (i = 0; i < len; i += 3) {
> to
> for (i = 0; i < len - 2; i += 3) {
>
> for the return, I think it's correct to return the used length,
> if it's error, have return already. right?


I doubt so.

>
>
> --
> Thanks,
> Limin Wang
> _______________________________________________
> 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".
>
Lance Wang May 12, 2022, 3:17 p.m. UTC | #4 
On Thu, May 12, 2022 at 08:25:29AM +0200, Paul B Mahol wrote:
> On Thu, May 12, 2022 at 1:39 AM <lance.lmwang@gmail.com> wrote:
> 
> > On Wed, May 11, 2022 at 09:47:52PM +0200, Paul B Mahol wrote:
> > > why?
> >
> > assuming the len is 1, the following code will access the next 3
> > array anymore, I think it's better to check the i with len -2.
> >
> > for (i = 0; i < len; i += 3) {
> > to
> > for (i = 0; i < len - 2; i += 3) {
> >
> > for the return, I think it's correct to return the used length,
> > if it's error, have return already. right?
> 
> 
> I doubt so.

maybe I'm misunderstand it, but from the comments, the API claims that:
libavcodec/codec_internal.h
175          * @return amount of bytes read from the packet on success,
176          *         negative error code on failure
177          */
178         int (*decode)(struct AVCodecContext *avctx, struct AVFrame *frame,
179                       int *got_frame_ptr, struct AVPacket *avpkt);
180         /**
181          * Decode subtitle data to an AVSubtitle.
182          * cb is in this state if cb_type is FF_CODEC_CB_TYPE_DECODE_SUB.
183          *
184          * Apart from that this is like the decode callback.
185          */
186         int (*decode_sub)(struct AVCodecContext *avctx, struct AVSubtitle *sub,
187                           int *got_frame_ptr, const struct AVPacket *avpkt);

> >
> >
> > --
> > Thanks,
> > Limin Wang
> > _______________________________________________
> > 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".
Andreas Rheinhardt May 12, 2022, 3:30 p.m. UTC | #5 
lance.lmwang@gmail.com:
> On Thu, May 12, 2022 at 08:25:29AM +0200, Paul B Mahol wrote:
>> On Thu, May 12, 2022 at 1:39 AM <lance.lmwang@gmail.com> wrote:
>>
>>> On Wed, May 11, 2022 at 09:47:52PM +0200, Paul B Mahol wrote:
>>>> why?
>>>
>>> assuming the len is 1, the following code will access the next 3
>>> array anymore, I think it's better to check the i with len -2.
>>>
>>> for (i = 0; i < len; i += 3) {
>>> to
>>> for (i = 0; i < len - 2; i += 3) {
>>>
>>> for the return, I think it's correct to return the used length,
>>> if it's error, have return already. right?
>>
>>
>> I doubt so.
> 
> maybe I'm misunderstand it, but from the comments, the API claims that:
> libavcodec/codec_internal.h
> 175          * @return amount of bytes read from the packet on success,
> 176          *         negative error code on failure
> 177          */
> 178         int (*decode)(struct AVCodecContext *avctx, struct AVFrame *frame,
> 179                       int *got_frame_ptr, struct AVPacket *avpkt);
> 180         /**
> 181          * Decode subtitle data to an AVSubtitle.
> 182          * cb is in this state if cb_type is FF_CODEC_CB_TYPE_DECODE_SUB.
> 183          *
> 184          * Apart from that this is like the decode callback.
> 185          */
> 186         int (*decode_sub)(struct AVCodecContext *avctx, struct AVSubtitle *sub,
> 187                           int *got_frame_ptr, const struct AVPacket *avpkt);
> 

This is correct. It is not only the internal API which claims that, but
the public API, too. And this just not honoured, in particular not in
case of subtitle recoding. See
https://github.com/mkver/FFmpeg/commit/ba1564c532654888015d67b70bf73d117c2d9f75

- Andreas
Andreas Rheinhardt May 12, 2022, 4:29 p.m. UTC | #6 
Andreas Rheinhardt:
> lance.lmwang@gmail.com:
>> On Thu, May 12, 2022 at 08:25:29AM +0200, Paul B Mahol wrote:
>>> On Thu, May 12, 2022 at 1:39 AM <lance.lmwang@gmail.com> wrote:
>>>
>>>> On Wed, May 11, 2022 at 09:47:52PM +0200, Paul B Mahol wrote:
>>>>> why?
>>>>
>>>> assuming the len is 1, the following code will access the next 3
>>>> array anymore, I think it's better to check the i with len -2.
>>>>
>>>> for (i = 0; i < len; i += 3) {
>>>> to
>>>> for (i = 0; i < len - 2; i += 3) {
>>>>
>>>> for the return, I think it's correct to return the used length,
>>>> if it's error, have return already. right?
>>>
>>>
>>> I doubt so.
>>
>> maybe I'm misunderstand it, but from the comments, the API claims that:
>> libavcodec/codec_internal.h
>> 175          * @return amount of bytes read from the packet on success,
>> 176          *         negative error code on failure
>> 177          */
>> 178         int (*decode)(struct AVCodecContext *avctx, struct AVFrame *frame,
>> 179                       int *got_frame_ptr, struct AVPacket *avpkt);
>> 180         /**
>> 181          * Decode subtitle data to an AVSubtitle.
>> 182          * cb is in this state if cb_type is FF_CODEC_CB_TYPE_DECODE_SUB.
>> 183          *
>> 184          * Apart from that this is like the decode callback.
>> 185          */
>> 186         int (*decode_sub)(struct AVCodecContext *avctx, struct AVSubtitle *sub,
>> 187                           int *got_frame_ptr, const struct AVPacket *avpkt);
>>
> 
> This is correct. It is not only the internal API which claims that, but
> the public API, too. And this just not honoured, in particular not in
> case of subtitle recoding. See
> https://github.com/mkver/FFmpeg/commit/ba1564c532654888015d67b70bf73d117c2d9f75
> 

It seems like there really are people who call this in a loop until all
the input is exhausted (as the documentation leads you to believe (The
internal uses of avcodec_decode_subtitle2 don't do this.)):
https://github.com/HandBrake/HandBrake/blob/a40fb6bce6755209461166140f131f26a2857eb9/libhb/decavsub.c#L335
I wonder if they ever got something meaningful the second time they
called it with the same packet (presuming there was a second time).

- Andreas
Lance Wang May 13, 2022, 2:10 a.m. UTC | #7 
On Thu, May 12, 2022 at 06:29:51PM +0200, Andreas Rheinhardt wrote:
> Andreas Rheinhardt:
> > lance.lmwang@gmail.com:
> >> On Thu, May 12, 2022 at 08:25:29AM +0200, Paul B Mahol wrote:
> >>> On Thu, May 12, 2022 at 1:39 AM <lance.lmwang@gmail.com> wrote:
> >>>
> >>>> On Wed, May 11, 2022 at 09:47:52PM +0200, Paul B Mahol wrote:
> >>>>> why?
> >>>>
> >>>> assuming the len is 1, the following code will access the next 3
> >>>> array anymore, I think it's better to check the i with len -2.
> >>>>
> >>>> for (i = 0; i < len; i += 3) {
> >>>> to
> >>>> for (i = 0; i < len - 2; i += 3) {
> >>>>
> >>>> for the return, I think it's correct to return the used length,
> >>>> if it's error, have return already. right?
> >>>
> >>>
> >>> I doubt so.
> >>
> >> maybe I'm misunderstand it, but from the comments, the API claims that:
> >> libavcodec/codec_internal.h
> >> 175          * @return amount of bytes read from the packet on success,
> >> 176          *         negative error code on failure
> >> 177          */
> >> 178         int (*decode)(struct AVCodecContext *avctx, struct AVFrame *frame,
> >> 179                       int *got_frame_ptr, struct AVPacket *avpkt);
> >> 180         /**
> >> 181          * Decode subtitle data to an AVSubtitle.
> >> 182          * cb is in this state if cb_type is FF_CODEC_CB_TYPE_DECODE_SUB.
> >> 183          *
> >> 184          * Apart from that this is like the decode callback.
> >> 185          */
> >> 186         int (*decode_sub)(struct AVCodecContext *avctx, struct AVSubtitle *sub,
> >> 187                           int *got_frame_ptr, const struct AVPacket *avpkt);
> >>
> > 
> > This is correct. It is not only the internal API which claims that, but
> > the public API, too. And this just not honoured, in particular not in
> > case of subtitle recoding. See
> > https://github.com/mkver/FFmpeg/commit/ba1564c532654888015d67b70bf73d117c2d9f75
> > 
> 
> It seems like there really are people who call this in a loop until all
> the input is exhausted (as the documentation leads you to believe (The
> internal uses of avcodec_decode_subtitle2 don't do this.)):
> https://github.com/HandBrake/HandBrake/blob/a40fb6bce6755209461166140f131f26a2857eb9/libhb/decavsub.c#L335
> I wonder if they ever got something meaningful the second time they
> called it with the same packet (presuming there was a second time).

At first, I thought it was an unintentional return as all other subtitle decode
return packet size always. If the code don't support for that as claims by document,
then I prefer to fix the document. If not, we need to fix the code.


> 
> - 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/libavcodec/ccaption_dec.c b/libavcodec/ccaption_dec.c
index 34f0513b1a..8f61e8aa03 100644
--- a/libavcodec/ccaption_dec.c
+++ b/libavcodec/ccaption_dec.c
@@ -852,6 +852,11 @@  static int decode(AVCodecContext *avctx, AVSubtitle *sub,
     int i;
     unsigned nb_rect_allocated = 0;
 
+    if (len < 3) {
+        ff_dlog(avctx, "incomplete or broken packet");
+        return len;
+    }
+
     for (i = 0; i < len; i += 3) {
         uint8_t hi, cc_type = bptr[i] & 1;
 
@@ -922,7 +927,7 @@  static int decode(AVCodecContext *avctx, AVSubtitle *sub,
     }
 
     *got_sub = sub->num_rects > 0;
-    return ret;
+    return len;
 }
 
 #define OFFSET(x) offsetof(CCaptionSubContext, x)