[FFmpeg-devel] [PATCH] avformat/mov: Fix get_eia608_packet c608 caption reformatting

[FFmpeg-devel] [PATCH] avformat/mov: Fix get_eia608_packet c608 caption reformatting

[Pavel Koshevoy]

---
 libavformat/mov.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 85aef33b19..422e515fe8 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -10790,22 +10790,44 @@ static int mov_change_extradata(AVStream *st, AVPacket *pkt)
 
 static int get_eia608_packet(AVIOContext *pb, AVPacket *pkt, int size)
 {
-    int new_size, ret;
-
+    /* We can't make assumptions about the structure of the payload,
+       because it may include multiple cdat and cdt2 samples. */
+    int ret, out_size = 0;
     if (size <= 8)
         return AVERROR_INVALIDDATA;
-    new_size = ((size - 8) / 2) * 3;
-    ret = av_new_packet(pkt, new_size);
+
+    /* Allocate 3 times the required buffer size to give us enough space
+       to store the original c608 packet and (potentially 3/2 times larger)
+       reformatted data, safely. */
+    ret = av_new_packet(pkt, size * 3);
     if (ret < 0)
         return ret;
 
-    avio_skip(pb, 8);
-    for (int j = 0; j < new_size; j += 3) {
-        pkt->data[j] = 0xFC;
-        pkt->data[j+1] = avio_r8(pb);
-        pkt->data[j+2] = avio_r8(pb);
+    /* Load the original c608 payload into the last 3rd of the buffer. */
+    if (avio_read(pb, pkt->data + size * 2, size) != size)
+        return AVERROR_EOF;
+
+    /* parse and re-format the c608 payload in one pass */
+    for (uint8_t *out = pkt->data, *src = out + size * 2, *end = src + size; src + 8 <= end; ) {
+        uint32_t atom_size = AV_RB32(src);
+        uint8_t cc_field =
+            memcmp(src + 4, "cdat", 4) == 0 ? 1 :
+            memcmp(src + 4, "cdt2", 4) == 0 ? 2 :
+            0;
+
+        if (cc_field != 0) {
+            for (uint8_t *cc = src + 8, *cc_end = src + atom_size; cc < cc_end; cc += 2) {
+                out[0] = (0x1F << 3) | (1 << 2) | (cc_field - 1);
+                out[1] = cc[0];
+                out[2] = cc[1];
+                out_size += 3;
+                out += 3;
+            }
+        }
+        src += atom_size;
     }
 
+    pkt->size = out_size;
     return 0;
 }
 
-- 
2.43.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".

Re: [FFmpeg-devel] [PATCH] avformat/mov: Fix get_eia608_packet c608 caption reformatting

[Pavel Koshevoy]

BTW, the problem is reproducible with "Test for Quicktime 608 CC file.mov"
from https://samples.ffmpeg.org/MPEG2/subcc/

Pavel.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avformat/mov: Fix get_eia608_packet c608 caption reformatting

[Andreas Rheinhardt]

Pavel Koshevoy:
> ---
>  libavformat/mov.c | 40 +++++++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 85aef33b19..422e515fe8 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -10790,22 +10790,44 @@ static int mov_change_extradata(AVStream *st, AVPacket *pkt)
>  
>  static int get_eia608_packet(AVIOContext *pb, AVPacket *pkt, int size)
>  {
> -    int new_size, ret;
> -
> +    /* We can't make assumptions about the structure of the payload,
> +       because it may include multiple cdat and cdt2 samples. */
> +    int ret, out_size = 0;
>      if (size <= 8)
>          return AVERROR_INVALIDDATA;
> -    new_size = ((size - 8) / 2) * 3;
> -    ret = av_new_packet(pkt, new_size);
> +
> +    /* Allocate 3 times the required buffer size to give us enough space
> +       to store the original c608 packet and (potentially 3/2 times larger)
> +       reformatted data, safely. */
> +    ret = av_new_packet(pkt, size * 3);
>      if (ret < 0)
>          return ret;
>  
> -    avio_skip(pb, 8);
> -    for (int j = 0; j < new_size; j += 3) {
> -        pkt->data[j] = 0xFC;
> -        pkt->data[j+1] = avio_r8(pb);
> -        pkt->data[j+2] = avio_r8(pb);
> +    /* Load the original c608 payload into the last 3rd of the buffer. */
> +    if (avio_read(pb, pkt->data + size * 2, size) != size)
> +        return AVERROR_EOF;
> +
> +    /* parse and re-format the c608 payload in one pass */
> +    for (uint8_t *out = pkt->data, *src = out + size * 2, *end = src + size; src + 8 <= end; ) {
> +        uint32_t atom_size = AV_RB32(src);
> +        uint8_t cc_field =
> +            memcmp(src + 4, "cdat", 4) == 0 ? 1 :
> +            memcmp(src + 4, "cdt2", 4) == 0 ? 2 :
> +            0;
> +
> +        if (cc_field != 0) {
> +            for (uint8_t *cc = src + 8, *cc_end = src + atom_size; cc < cc_end; cc += 2) {
> +                out[0] = (0x1F << 3) | (1 << 2) | (cc_field - 1);
> +                out[1] = cc[0];
> +                out[2] = cc[1];
> +                out_size += 3;
> +                out += 3;
> +            }
> +        }
> +        src += atom_size;
>      }
>  
> +    pkt->size = out_size;
>      return 0;
>  }
>  

1. You are trusting atom_size, although it may be insanely large. This
may lead to segfaults.
2. 3 * size might overflow.
3. You overallocate a lot here. Wouldn't it be enough to allocate size +
size / 2? You would then read into pkt->data + size / 2 and reformat the
data from the start.
4. You need to shrink the packet to zero the padding.
5. You don't need to keep track of out_size; you can get it via out -
pkt->data.

- 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".

Re: [FFmpeg-devel] [PATCH] avformat/mov: Fix get_eia608_packet c608 caption reformatting

[Pavel Koshevoy]

On Wed, Feb 12, 2025 at 11:47 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> Pavel Koshevoy:
> > ---
> >  libavformat/mov.c | 40 +++++++++++++++++++++++++++++++---------
> >  1 file changed, 31 insertions(+), 9 deletions(-)
> >
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index 85aef33b19..422e515fe8 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -10790,22 +10790,44 @@ static int mov_change_extradata(AVStream *st,
> AVPacket *pkt)
> >
> >  static int get_eia608_packet(AVIOContext *pb, AVPacket *pkt, int size)
> >  {
> > -    int new_size, ret;
> > -
> > +    /* We can't make assumptions about the structure of the payload,
> > +       because it may include multiple cdat and cdt2 samples. */
> > +    int ret, out_size = 0;
> >      if (size <= 8)
> >          return AVERROR_INVALIDDATA;
> > -    new_size = ((size - 8) / 2) * 3;
> > -    ret = av_new_packet(pkt, new_size);
> > +
> > +    /* Allocate 3 times the required buffer size to give us enough space
> > +       to store the original c608 packet and (potentially 3/2 times
> larger)
> > +       reformatted data, safely. */
> > +    ret = av_new_packet(pkt, size * 3);
> >      if (ret < 0)
> >          return ret;
> >
> > -    avio_skip(pb, 8);
> > -    for (int j = 0; j < new_size; j += 3) {
> > -        pkt->data[j] = 0xFC;
> > -        pkt->data[j+1] = avio_r8(pb);
> > -        pkt->data[j+2] = avio_r8(pb);
> > +    /* Load the original c608 payload into the last 3rd of the buffer.
> */
> > +    if (avio_read(pb, pkt->data + size * 2, size) != size)
> > +        return AVERROR_EOF;
> > +
> > +    /* parse and re-format the c608 payload in one pass */
> > +    for (uint8_t *out = pkt->data, *src = out + size * 2, *end = src +
> size; src + 8 <= end; ) {
> > +        uint32_t atom_size = AV_RB32(src);
> > +        uint8_t cc_field =
> > +            memcmp(src + 4, "cdat", 4) == 0 ? 1 :
> > +            memcmp(src + 4, "cdt2", 4) == 0 ? 2 :
> > +            0;
> > +
> > +        if (cc_field != 0) {
> > +            for (uint8_t *cc = src + 8, *cc_end = src + atom_size; cc <
> cc_end; cc += 2) {
> > +                out[0] = (0x1F << 3) | (1 << 2) | (cc_field - 1);
> > +                out[1] = cc[0];
> > +                out[2] = cc[1];
> > +                out_size += 3;
> > +                out += 3;
> > +            }
> > +        }
> > +        src += atom_size;
> >      }
> >
> > +    pkt->size = out_size;
> >      return 0;
> >  }
> >
>
> 1. You are trusting atom_size, although it may be insanely large. This
> may lead to segfaults.
> 2. 3 * size might overflow.
> 3. You overallocate a lot here. Wouldn't it be enough to allocate size +
> size / 2? You would then read into pkt->data + size / 2 and reformat the
> data from the start.
> 4. You need to shrink the packet to zero the padding.
> 5. You don't need to keep track of out_size; you can get it via out -
> pkt->data.
>
>
Thank you, I've submitted a 2nd version of the patch that performs more
input validation.

Pavel.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avformat/mov: Fix get_eia608_packet c608 caption reformatting

[Devin Heitmueller]

On Thu, Feb 13, 2025 at 12:57 PM Pavel Koshevoy <pkoshevoy@gmail.com> wrote:
> Thank you, I've submitted a 2nd version of the patch that performs more
> input validation.

For what it's worth, a patch was submitted nearly two years ago which
achieves the same goal (never upstreamed).  You may wish to make sure
it covers the cases you care about:

https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2023-June/310456.html

Devin

-- 
Devin Heitmueller, Senior Software Engineer
LTN Global Communications
o: +1 (301) 363-1001
w: https://ltnglobal.com  e: devin.heitmueller@ltnglobal.com
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avformat/mov: Fix get_eia608_packet c608 caption reformatting

[Pavel Koshevoy]

On Thu, Feb 13, 2025 at 12:02 PM Devin Heitmueller <
devin.heitmueller@ltnglobal.com> wrote:

> On Thu, Feb 13, 2025 at 12:57 PM Pavel Koshevoy <pkoshevoy@gmail.com>
> wrote:
> > Thank you, I've submitted a 2nd version of the patch that performs more
> > input validation.
>
> For what it's worth, a patch was submitted nearly two years ago which
> achieves the same goal (never upstreamed).  You may wish to make sure
> it covers the cases you care about:
>
> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2023-June/310456.html
>
>
yes, that patch is better in that it doesn't over-allocate.
I don't care much which fix gets merged, so long as the problem is fixed.

Thank you,
    Pavel.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avformat/mov: Fix get_eia608_packet c608 caption reformatting

[Devin Heitmueller]

On Thu, Feb 13, 2025 at 3:00 PM Pavel Koshevoy <pkoshevoy@gmail.com> wrote:
> yes, that patch is better in that it doesn't over-allocate.
> I don't care much which fix gets merged, so long as the problem is fixed.

Ok, if you could please try that patch and assuming it works for you
reply to the list accordingly.  I tried it two years ago (which is how
I found a bug in it), but it would be good to have someone else test
it before it gets merged.

Thanks,

Devin

-- 
Devin Heitmueller, Senior Software Engineer
LTN Global Communications
o: +1 (301) 363-1001
w: https://ltnglobal.com  e: devin.heitmueller@ltnglobal.com
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avformat/mov: Fix get_eia608_packet c608 caption reformatting

[Pavel Koshevoy]

On Thu, Feb 13, 2025 at 1:12 PM Devin Heitmueller <
devin.heitmueller@ltnglobal.com> wrote:

> On Thu, Feb 13, 2025 at 3:00 PM Pavel Koshevoy <pkoshevoy@gmail.com>
> wrote:
> > yes, that patch is better in that it doesn't over-allocate.
> > I don't care much which fix gets merged, so long as the problem is fixed.
>
> Ok, if you could please try that patch and assuming it works for you
> reply to the list accordingly.  I tried it two years ago (which is how
> I found a bug in it), but it would be good to have someone else test
> it before it gets merged.



I've submitted a 3rd version of the patch that doesn't do as much
over-allocation, similar to
https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2023-June/310456.html
My v3 patch works correctly with the 2 sample files I can test with.

Pavel.
_______________________________________________
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".