Re: [FFmpeg-devel] [FFmpeg-cvslog] avcodec/magicyuv: add vlc multi support

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 7910 bytes --]

On Sat, Oct 21, 2023 at 11:00:19AM +0200, Paul B Mahol wrote:
> On Sat, Oct 21, 2023 at 2:13 AM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Wed, Sep 06, 2023 at 10:19:27PM +0000, Paul B Mahol wrote:
> > > ffmpeg | branch: master | Paul B Mahol <onemda@gmail.com> | Mon Aug 28
> > 12:20:15 2023 +0200| [8b7391cb5ff94ce94612fda69392a95d7ab1ffd0] |
> > committer: Paul B Mahol
> > >
> > > avcodec/magicyuv: add vlc multi support
> > >
> > > Gives nice speed boost, depending on encoded content it goes from
> > > 30% to 60% faster.
> > >
> > > >
> > http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=8b7391cb5ff94ce94612fda69392a95d7ab1ffd0
> > > ---
> > >
> > >  libavcodec/magicyuv.c | 65
> > +++++++++++++++++++++++++++------------------------
> > >  1 file changed, 34 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/libavcodec/magicyuv.c b/libavcodec/magicyuv.c
> > > index 7898cd5be4..bbaf14d0e0 100644
> > > --- a/libavcodec/magicyuv.c
> > > +++ b/libavcodec/magicyuv.c
> > > @@ -34,6 +34,8 @@
> > >  #include "lossless_videodsp.h"
> > >  #include "thread.h"
> > >
> > > +#define VLC_BITS 12
> > > +
> > >  typedef struct Slice {
> > >      uint32_t start;
> > >      uint32_t size;
> > > @@ -67,13 +69,14 @@ typedef struct MagicYUVContext {
> > >      Slice            *slices[4];      // slice bitstream positions for
> > each plane
> > >      unsigned int      slices_size[4]; // slice sizes for each plane
> > >      VLC               vlc[4];         // VLC for each plane
> > > +    VLC_MULTI         multi[4];       // Buffer for joint VLC data
> > >      int (*magy_decode_slice)(AVCodecContext *avctx, void *tdata,
> > >                               int j, int threadnr);
> > >      LLVidDSPContext   llviddsp;
> > >  } MagicYUVContext;
> > >
> > >  static int huff_build(const uint8_t len[], uint16_t codes_pos[33],
> > > -                      VLC *vlc, int nb_elems, void *logctx)
> > > +                      VLC *vlc, VLC_MULTI *multi, int nb_elems, void
> > *logctx)
> > >  {
> > >      HuffEntry he[4096];
> > >
> > > @@ -84,7 +87,8 @@ static int huff_build(const uint8_t len[], uint16_t
> > codes_pos[33],
> > >          he[--codes_pos[len[i]]] = (HuffEntry){ len[i], i };
> > >
> > >      ff_free_vlc(vlc);
> > > -    return ff_init_vlc_from_lengths(vlc, FFMIN(he[0].len, 12), nb_elems,
> > > +    ff_free_vlc_multi(multi);
> > > +    return ff_init_vlc_multi_from_lengths(vlc, multi, FFMIN(he[0].len,
> > VLC_BITS), nb_elems, nb_elems,
> > >                                      &he[0].len, sizeof(he[0]),
> > >                                      &he[0].sym, sizeof(he[0]),
> > sizeof(he[0].sym),
> > >                                      0, 0, logctx);
> > > @@ -111,6 +115,22 @@ static void magicyuv_median_pred16(uint16_t *dst,
> > const uint16_t *src1,
> > >      *left_top = lt;
> > >  }
> > >
> > > +#define READ_PLANE(dst, plane, b, c) \
> > > +{ \
> > > +    x = 0; \
> > > +    for (; CACHED_BITSTREAM_READER && x < width-c && get_bits_left(&gb)
> > > 0;) {\
> > > +        ret = get_vlc_multi(&gb, (uint8_t *)dst + x * b, multi, \
> > > +                            vlc, vlc_bits, 3); \
> > > +        if (ret > 0) \
> > > +            x += ret; \
> > > +        if (ret <= 0) \
> > > +            return AVERROR_INVALIDDATA; \
> > > +    } \
> > > +    for (; x < width && get_bits_left(&gb) > 0; x++) \
> > > +        dst[x] = get_vlc2(&gb, vlc, vlc_bits, 3); \
> > > +    dst += stride; \
> > > +}
> > > +
> > >  static int magy_decode_slice10(AVCodecContext *avctx, void *tdata,
> > >                                 int j, int threadnr)
> > >  {
> > > @@ -130,6 +150,9 @@ static int magy_decode_slice10(AVCodecContext
> > *avctx, void *tdata,
> > >          int sheight = AV_CEIL_RSHIFT(s->slice_height, s->vshift[i]);
> > >          ptrdiff_t fake_stride = (p->linesize[i] / 2) * (1 + interlaced);
> > >          ptrdiff_t stride = p->linesize[i] / 2;
> > > +        const VLC_MULTI_ELEM *const multi = s->multi[i].table;
> > > +        const VLCElem *const vlc = s->vlc[i].table;
> > > +        const int vlc_bits = s->vlc[i].bits;
> > >          int flags, pred;
> > >          int ret = init_get_bits8(&gb, s->buf + s->slices[i][j].start,
> > >                                   s->slices[i][j].size);
> > > @@ -151,20 +174,8 @@ static int magy_decode_slice10(AVCodecContext
> > *avctx, void *tdata,
> > >                  dst += stride;
> > >              }
> > >          } else {
> > > -            for (k = 0; k < height; k++) {
> > > -                for (x = 0; x < width; x++) {
> > > -                    int pix;
> > > -                    if (get_bits_left(&gb) <= 0)
> > > -                        return AVERROR_INVALIDDATA;
> > > -
> > > -                    pix = get_vlc2(&gb, s->vlc[i].table,
> > s->vlc[i].bits, 3);
> > > -                    if (pix < 0)
> > > -                        return AVERROR_INVALIDDATA;
> > > -
> > > -                    dst[x] = pix;
> > > -                }
> > > -                dst += stride;
> > > -            }
> > > +            for (k = 0; k < height; k++)
> > > +                READ_PLANE(dst, i, 2, 3)
> > >          }
> > >
> > >          switch (pred) {
> > > @@ -261,6 +272,9 @@ static int magy_decode_slice(AVCodecContext *avctx,
> > void *tdata,
> > >          ptrdiff_t fake_stride = p->linesize[i] * (1 + interlaced);
> > >          ptrdiff_t stride = p->linesize[i];
> > >          const uint8_t *slice = s->buf + s->slices[i][j].start;
> > > +        const VLC_MULTI_ELEM *const multi = s->multi[i].table;
> > > +        const VLCElem *const vlc = s->vlc[i].table;
> > > +        const int vlc_bits = s->vlc[i].bits;
> > >          int flags, pred;
> > >
> > >          flags = bytestream_get_byte(&slice);
> > > @@ -280,20 +294,8 @@ static int magy_decode_slice(AVCodecContext *avctx,
> > void *tdata,
> > >              if (ret < 0)
> > >                  return ret;
> > >
> > > -            for (k = 0; k < height; k++) {
> > > -                for (x = 0; x < width; x++) {
> > > -                    int pix;
> > > -                    if (get_bits_left(&gb) <= 0)
> > > -                        return AVERROR_INVALIDDATA;
> > > -
> > > -                    pix = get_vlc2(&gb, s->vlc[i].table,
> > s->vlc[i].bits, 3);
> > > -                    if (pix < 0)
> > > -                        return AVERROR_INVALIDDATA;
> > > -
> > > -                    dst[x] = pix;
> > > -                }
> > > -                dst += stride;
> > > -            }
> > > +            for (k = 0; k < height; k++)
> > > +                READ_PLANE(dst, i, 1, 5)
> > >          }
> > >
> > >          switch (pred) {
> >
> > Who reviewed this ?
> >
> > This is a straight out of array write
> > writing 8 bytes while the check assumes its max 5
> >
> 
> Why are you becoming so aggressive?

I objected to this patchset because of copy and pasted (duplicated) code,
you ignored this objection and pushed it anyway. Now its found out that
it contains a out of array write. After a DOS vulnerability was found
previosuly in it.
You expect me to be happy ?


>
> The function writes at once 8 bytes, even though only 6 values (in 8bit
> case) are kept.
> 

> Can you share sample?

I will mail you what i have but the data is not in a container for ffmpeg
it needs the fuzzer to replicate. I think if you just fix the check to match
the amount of data written that would fix this case. Of course neither that
fix nor the sample will do anything if there are other issues.

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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