On Thu, Sep 06, 2018 at 11:34:57AM +0530, Gagandeep Singh wrote:
> On Sat, Aug 18, 2018 at 1:47 AM Michael Niedermayer <mich...@niedermayer.cc>
> wrote:
>
> > On Fri, Aug 17, 2018 at 11:45:04AM +0530, Gagandeep Singh wrote:
> > [...]
> > >
> > > > [...]
> > > > > @@ -726,14 +814,15 @@ static int cfhd_decode(AVCodecContext *avctx,
> > void
> > > > *data, int *got_frame,
> > > > > }
> > > > > }
> > > > > }
> > > > > -
> > > > > - if (!s->a_width || !s->a_height || s->a_format ==
> > AV_PIX_FMT_NONE ||
> > > > > - s->coded_width || s->coded_height || s->coded_format !=
> > > > AV_PIX_FMT_NONE) {
> > > >
> > > > > + //disabled to run mountain sample file
> > > > > +#if 0
> > > > > + if ((!s->a_width || !s->a_height || s->a_format ==
> > AV_PIX_FMT_NONE
> > > > ||
> > > > > + s->coded_width || s->coded_height || s->coded_format !=
> > > > AV_PIX_FMT_NONE) && s->sample_type != 1) {
> > > > > av_log(avctx, AV_LOG_ERROR, "Invalid dimensions\n");
> > > > > ret = AVERROR(EINVAL);
> > > > > goto end;
> > > > > }
> > > > > -
> > > > > +#endif
> > > >
> > > > please elaborate why this needs to be disabled
> > > > i presume this code was needed for something
> > > >
> > > I didn't need to disable this for any sample except one, where the image
> > > height and width data wasn't transfered in accordance to how it was in
> > the
> > > rest of the sample so the flow of code was just causing the decoder to
> > > crash. I can produce a more robust fix, though again will need to repost
> > > other patches as well, please comment.
> > >
> > > > also this decoder with the patches should be tested with a fuzzer to
> > > > reduce
> > > > the chance of bugs
> > > >
> > > > I don't know how to use 'fuzzer', sorry, though i can look into that.
> >
> > had missed this reply as its not quoted correctly
> > yes, please look into testing this with a fuzzer, we should make reasonable
> > sure we dont add anomalies
> >
> > thx
> >
> > [...]
> > --
> > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > Dictatorship naturally arises out of democracy, and the most aggravated
> > form of tyranny and slavery out of the most extreme liberty. -- Plato
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
> Hi,
> patch updated to add back the check.
> cfhd.c | 504
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
> cfhd.h | 13 +
> 2 files changed, 450 insertions(+), 67 deletions(-)
> 67182ed6c7222c3a544f07d3501c072da16682ae 3d_transform_add.patch
> From f99726110a9e9e127fc2c7b4bbc3945764ecbfdd Mon Sep 17 00:00:00 2001
> From: Gagandeep Singh <deepgagan231...@gmail.com>
> Date: Thu, 6 Sep 2018 11:08:57 +0530
> Subject: [GSOC][PATCH 1/3] lavc/cfhd: 3d transform decoding added for both
> progressive and interlaced samples
>
> ---
> libavcodec/cfhd.c | 504 ++++++++++++++++++++++++++++++++++++++++------
> libavcodec/cfhd.h | 13 +-
> 2 files changed, 450 insertions(+), 67 deletions(-)
>
> diff --git a/libavcodec/cfhd.c b/libavcodec/cfhd.c
> index 846d334b9b..3929b54f31 100644
> --- a/libavcodec/cfhd.c
> +++ b/libavcodec/cfhd.c
> @@ -41,12 +41,15 @@
> #define ALPHA_COMPAND_GAIN 9400
>
> enum CFHDParam {
> + TransformType = 10,
> ChannelCount = 12,
> SubbandCount = 14,
> + Pframe = 19,
> ImageWidth = 20,
> ImageHeight = 21,
> LowpassPrecision = 35,
> SubbandNumber = 48,
> + EncodingMethod = 52,
> Quantization = 53,
> ChannelNumber = 62,
> SampleFlags = 68,
> @@ -64,6 +67,7 @@ static av_cold int cfhd_init(AVCodecContext *avctx)
>
> avctx->bits_per_raw_sample = 10;
> s->avctx = avctx;
> + s->progressive = 0;
>
> return ff_cfhd_init_vlcs(s);
> }
> @@ -84,6 +88,10 @@ static void init_peak_table_defaults(CFHDContext *s)
>
> static void init_frame_defaults(CFHDContext *s)
> {
> + s->sample_type = 0;
> + s->transform_type = 0;
> + s->pframe = 0;
> + s->first_wavelet = 0;
> s->coded_width = 0;
> s->coded_height = 0;
> s->cropped_height = 0;
> @@ -97,14 +105,15 @@ static void init_frame_defaults(CFHDContext *s)
> s->pshift = 1;
> s->codebook = 0;
> s->difference_coding = 0;
> - s->progressive = 0;
> init_plane_defaults(s);
> init_peak_table_defaults(s);
> }
>
> /* TODO: merge with VLC tables or use LUT */
> -static inline int dequant_and_decompand(int level, int quantisation, int
> codebook)
> +static inline int dequant_and_decompand(int level, int quantisation, int
> codebook, int lossless)
> {
> + if (lossless)
> + return level;
> if (codebook == 0 || codebook == 1) {
> int64_t abslevel = abs(level);
> if (level < 264)
> @@ -193,16 +202,21 @@ static inline void filter(int16_t *output, ptrdiff_t
> out_stride,
> }
> }
>
> -static inline void interlaced_vertical_filter(int16_t *output, int16_t *low,
> int16_t *high,
> - int width, int linesize, int plane)
> +static inline void inverse_temporal_filter(int16_t *output, int16_t *low,
> int16_t *high,
> + int width, int linesize, int temporal_for_highpass)
> {
> int i;
> int16_t even, odd;
> for (i = 0; i < width; i++) {
> even = (low[i] - high[i])/2;
> odd = (low[i] + high[i])/2;
> - output[i] = av_clip_uintp2(even, 10);
> - output[i + linesize] = av_clip_uintp2(odd, 10);
> + if (!temporal_for_highpass) {
> + output[i] = av_clip_uintp2(even, 10);
> + output[i + linesize] = av_clip_uintp2(odd, 10);
> + } else {
> + low[i] = even;
> + high[i] = odd;
> + }
> }
> }
> static void horiz_filter(int16_t *output, int16_t *low, int16_t *high,
> @@ -231,9 +245,12 @@ static void free_buffers(CFHDContext *s)
> for (i = 0; i < FF_ARRAY_ELEMS(s->plane); i++) {
> av_freep(&s->plane[i].idwt_buf);
> av_freep(&s->plane[i].idwt_tmp);
> -
> - for (j = 0; j < 9; j++)
> - s->plane[i].subband[j] = NULL;
> + if (s->transform_type == 0)
> + for (j = 0; j < 9; j++)
> + s->plane[i].subband[j] = NULL;
> + else
> + for (j = 0; j < 17; j++)
> + s->plane[i].subband[j] = NULL;
>
> for (j = 0; j < 8; j++)
> s->plane[i].l_h[j] = NULL;
> @@ -247,7 +264,7 @@ static int alloc_buffers(AVCodecContext *avctx)
> CFHDContext *s = avctx->priv_data;
> int i, j, ret, planes;
> int chroma_x_shift, chroma_y_shift;
> - unsigned k;
> + unsigned k, t;
>
> if ((ret = ff_set_dimensions(avctx, s->coded_width, s->coded_height)) <
> 0)
> return ret;
> @@ -261,6 +278,7 @@ static int alloc_buffers(AVCodecContext *avctx)
>
> for (i = 0; i < planes; i++) {
> int w8, h8, w4, h4, w2, h2;
> + int16_t *frame2;
> int width = i ? avctx->width >> chroma_x_shift : avctx->width;
> int height = i ? avctx->height >> chroma_y_shift : avctx->height;
> ptrdiff_t stride = FFALIGN(width / 8, 8) * 8;
> @@ -277,28 +295,68 @@ static int alloc_buffers(AVCodecContext *avctx)
> w2 = w4 * 2;
> h2 = h4 * 2;
>
> - s->plane[i].idwt_buf =
> - av_mallocz_array(height * stride, sizeof(*s->plane[i].idwt_buf));
> - s->plane[i].idwt_tmp =
> - av_malloc_array(height * stride, sizeof(*s->plane[i].idwt_tmp));
> - if (!s->plane[i].idwt_buf || !s->plane[i].idwt_tmp)
> - return AVERROR(ENOMEM);
> -
> - s->plane[i].subband[0] = s->plane[i].idwt_buf;
> - s->plane[i].subband[1] = s->plane[i].idwt_buf + 2 * w8 * h8;
> - s->plane[i].subband[2] = s->plane[i].idwt_buf + 1 * w8 * h8;
> - s->plane[i].subband[3] = s->plane[i].idwt_buf + 3 * w8 * h8;
> - s->plane[i].subband[4] = s->plane[i].idwt_buf + 2 * w4 * h4;
> - s->plane[i].subband[5] = s->plane[i].idwt_buf + 1 * w4 * h4;
> - s->plane[i].subband[6] = s->plane[i].idwt_buf + 3 * w4 * h4;
> - s->plane[i].subband[7] = s->plane[i].idwt_buf + 2 * w2 * h2;
> - s->plane[i].subband[8] = s->plane[i].idwt_buf + 1 * w2 * h2;
> - s->plane[i].subband[9] = s->plane[i].idwt_buf + 3 * w2 * h2;
> -
> - for (j = 0; j < DWT_LEVELS; j++) {
> - for (k = 0; k < FF_ARRAY_ELEMS(s->plane[i].band[j]); k++) {
> - s->plane[i].band[j][k].a_width = w8 << j;
> - s->plane[i].band[j][k].a_height = h8 << j;
> + if (s->transform_type == 0) {
> + s->plane[i].idwt_buf =
> + av_mallocz_array(height * stride,
> sizeof(*s->plane[i].idwt_buf));
> + s->plane[i].idwt_tmp =
> + av_malloc_array(height * stride,
> sizeof(*s->plane[i].idwt_tmp));
> + if (!s->plane[i].idwt_buf || !s->plane[i].idwt_tmp)
> + return AVERROR(ENOMEM);
> + } else if (s->transform_type == 2) {
> + s->plane[i].idwt_buf =
> + av_mallocz_array(2 * height * stride,
> sizeof(*s->plane[i].idwt_buf));
> + s->plane[i].idwt_tmp =
> + av_malloc_array(2 * height * stride,
> sizeof(*s->plane[i].idwt_tmp));
> + if (!s->plane[i].idwt_buf || !s->plane[i].idwt_tmp)
> + return AVERROR(ENOMEM);
> + }
> +
> + if (s->transform_type == 0) {
> + s->plane[i].subband[0] = s->plane[i].idwt_buf;
> + s->plane[i].subband[1] = s->plane[i].idwt_buf + 2 * w8 * h8;
> + s->plane[i].subband[2] = s->plane[i].idwt_buf + 1 * w8 * h8;
> + s->plane[i].subband[3] = s->plane[i].idwt_buf + 3 * w8 * h8;
> + s->plane[i].subband[4] = s->plane[i].idwt_buf + 2 * w4 * h4;
> + s->plane[i].subband[5] = s->plane[i].idwt_buf + 1 * w4 * h4;
> + s->plane[i].subband[6] = s->plane[i].idwt_buf + 3 * w4 * h4;
> + s->plane[i].subband[7] = s->plane[i].idwt_buf + 2 * w2 * h2;
> + s->plane[i].subband[8] = s->plane[i].idwt_buf + 1 * w2 * h2;
> + s->plane[i].subband[9] = s->plane[i].idwt_buf + 3 * w2 * h2;
> + } else if (s->transform_type == 2) {
> + s->plane[i].subband[0] = s->plane[i].idwt_buf;
> + s->plane[i].subband[1] = s->plane[i].idwt_buf + 2 * w8 * h8;
> + s->plane[i].subband[2] = s->plane[i].idwt_buf + 1 * w8 * h8;
> + s->plane[i].subband[3] = s->plane[i].idwt_buf + 3 * w8 * h8;
> + s->plane[i].subband[4] = s->plane[i].idwt_buf + 2 * w4 * h4;
> + s->plane[i].subband[5] = s->plane[i].idwt_buf + 1 * w4 * h4;
> + s->plane[i].subband[6] = s->plane[i].idwt_buf + 3 * w4 * h4;
> + frame2 =
> + s->plane[i].subband[7] = s->plane[i].idwt_buf + 4 * w2 * h2;
> + s->plane[i].subband[8] = frame2 + 2 * w4 * h4;
> + s->plane[i].subband[9] = frame2 + 1 * w4 * h4;
> + s->plane[i].subband[10] = frame2 + 3 * w4 * h4;
> + s->plane[i].subband[11] = frame2 + 2 * w2 * h2;
> + s->plane[i].subband[12] = frame2 + 1 * w2 * h2;
> + s->plane[i].subband[13] = frame2 + 3 * w2 * h2;
> + s->plane[i].subband[14] = s->plane[i].idwt_buf + 2 * w2 * h2;
> + s->plane[i].subband[15] = s->plane[i].idwt_buf + 1 * w2 * h2;
> + s->plane[i].subband[16] = s->plane[i].idwt_buf + 3 * w2 * h2;
> + }
> +
> + if (s->transform_type == 0) {
> + for (j = 0; j < DWT_LEVELS - 3; j++) {
> + for (k = 0; k < FF_ARRAY_ELEMS(s->plane[i].band[j]); k++) {
> + s->plane[i].band[j][k].a_width = w8 << j;
> + s->plane[i].band[j][k].a_height = h8 << j;
> + }
> + }
> + } else if (s->transform_type == 2) {
> + for (j = 0; j < DWT_LEVELS; j++) {
> + t = j < 1 ? 0 : (j < 3 ? 1 : 2);
> + for (k = 0; k < FF_ARRAY_ELEMS(s->plane[i].band[0]); k++) {
> + s->plane[i].band[j][k].a_width = w8 << t;
> + s->plane[i].band[j][k].a_height = h8 << t;
> + }
> }
> }
>
> @@ -311,6 +369,11 @@ static int alloc_buffers(AVCodecContext *avctx)
> // s->plane[i].l_h[5] = ll1;
> s->plane[i].l_h[6] = s->plane[i].idwt_tmp;
> s->plane[i].l_h[7] = s->plane[i].idwt_tmp + 2 * w2 * h2;
> + if (s->transform_type == 2) {
> + frame2 = s->plane[i].idwt_tmp + 4 * w2 * h2;
> + s->plane[i].l_h[8] = frame2;
> + s->plane[i].l_h[9] = frame2 + 2 * w2 * h2;
> + }
> }
>
> s->a_height = s->coded_height;
> @@ -349,6 +412,9 @@ static int cfhd_decode(AVCodecContext *avctx, void *data,
> int *got_frame,
> } else if (tag == SampleFlags) {
> av_log(avctx, AV_LOG_DEBUG, "Progressive?%"PRIu16"\n", data);
> s->progressive = data & 0x0001;
> + } else if (tag == Pframe) {
> + s->pframe = 1;
> + av_log(avctx, AV_LOG_DEBUG, "Frame type %"PRIu16"\n", data);
> } else if (tag == ImageWidth) {
> av_log(avctx, AV_LOG_DEBUG, "Width %"PRIu16"\n", data);
> s->coded_width = data;
> @@ -373,7 +439,7 @@ static int cfhd_decode(AVCodecContext *avctx, void *data,
> int *got_frame,
> }
> } else if (tag == SubbandCount) {
> av_log(avctx, AV_LOG_DEBUG, "Subband Count: %"PRIu16"\n", data);
> - if (data != SUBBAND_COUNT) {
> + if (data != 10 && data != 17) {
> av_log(avctx, AV_LOG_ERROR, "Subband Count of %"PRIu16" is
> unsupported\n", data);
> ret = AVERROR_PATCHWELCOME;
> break;
> @@ -405,7 +471,7 @@ static int cfhd_decode(AVCodecContext *avctx, void *data,
> int *got_frame,
> } else if (tag == 51) {
> av_log(avctx, AV_LOG_DEBUG, "Subband number actual %"PRIu16"\n",
> data);
> s->subband_num_actual = data;
> - if (s->subband_num_actual >= 10) {
> + if (s->subband_num_actual >= 17 && s->subband_num_actual != 255)
> {
> av_log(avctx, AV_LOG_ERROR, "Invalid subband number
> actual\n");
> ret = AVERROR(EINVAL);
> break;
> @@ -420,31 +486,39 @@ static int cfhd_decode(AVCodecContext *avctx, void
> *data, int *got_frame,
> s->prescale_shift[1] = (data >> 3) & 0x7;
> s->prescale_shift[2] = (data >> 6) & 0x7;
> av_log(avctx, AV_LOG_DEBUG, "Prescale shift (VC-5): %x\n", data);
> + } else if (tag == EncodingMethod) {
> + s->encode_method = data;
> + av_log(avctx, AV_LOG_DEBUG, "Encode Method for Subband %d :
> %x\n",s->subband_num_actual, data);
> } else if (tag == 27) {
> av_log(avctx, AV_LOG_DEBUG, "Lowpass width %"PRIu16"\n", data);
> - if (data < 3 || data >
> s->plane[s->channel_num].band[0][0].a_width) {
> + if (data < 3) {
> av_log(avctx, AV_LOG_ERROR, "Invalid lowpass width\n");
data was checked against a_width before, it is no longer after this change.
> ret = AVERROR(EINVAL);
> break;
> }
> + if (s->coded_width == 0)
> + s->coded_width = data << 3;
> s->plane[s->channel_num].band[0][0].width = data;
> s->plane[s->channel_num].band[0][0].stride = data;
this updates the width and stride unconditionally but sets the coded_width
conditionally.
all buffers are allocated based on s->coded_width & coded_height. How does
this work if only one is updated and the check is no longer there ?
I mean an attacker can just put 2 tag=27 and with the first set coded_width
anyway he wants and with the 2nd set band width & stride anyway he wants
i presume that would cause out of array accesses later unless iam missing
something
the same pattern occurs in other places of the patch
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
