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

[Pavel Koshevoy <pkoshevoy@gmail.com>]

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