Re: [FFmpeg-devel] [PATCH] mjpegdec: consider chroma subsampling in size check
On 06.12.2015 22:18, Michael Niedermayer wrote: > On Sun, Dec 06, 2015 at 06:56:35PM +0100, Andreas Cadhalpun wrote: >> mjpegdec.c | 11 --- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> a294ce9a780fdd710d3661bc201b0c72d30786d3 >> 0001-mjpegdec-consider-chroma-subsampling-in-size-check.patch >> From 7788195340e1d0e1206660f12f003f952da750a6 Mon Sep 17 00:00:00 2001 >> From: Andreas Cadhalpun >> Date: Wed, 2 Dec 2015 21:52:23 +0100 >> Subject: [PATCH] mjpegdec: consider chroma subsampling in size check >> >> If the chroma components are subsampled, smaller buffers are allocated >> for them. In that case the maximal block_offset for the chroma >> components is not as large as for the luma component. >> >> This fixes out of bounds writes causing segmentation faults or memory >> corruption. >> > > LGTM Pushed. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mjpegdec: consider chroma subsampling in size check
On Sun, Dec 06, 2015 at 06:56:35PM +0100, Andreas Cadhalpun wrote: > On 05.12.2015 04:02, Michael Niedermayer wrote: > > On Fri, Dec 04, 2015 at 03:14:21PM +0100, Andreas Cadhalpun wrote: > >> On 03.12.2015 15:48, Michael Niedermayer wrote: > >>> On Wed, Dec 02, 2015 at 10:00:13PM +0100, Andreas Cadhalpun wrote: > @@ -1293,14 +1296,16 @@ static int mjpeg_decode_scan(MJpegDecodeContext > *s, int nb_components, int Ah, > v = s->v_scount[i]; > x = 0; > y = 0; > +h_shift = c ? chroma_h_shift: 0; > +v_shift = c ? chroma_v_shift: 0; > for (j = 0; j < n; j++) { > block_offset = (((linesize[c] * (v * mb_y + y) * 8) > + > (h * mb_x + x) * 8 * > bytes_per_pixel) >> s->avctx->lowres); > > if (s->interlaced && s->bottom_field) > block_offset += linesize[c] >> 1; > -if ( 8*(h * mb_x + x) < s->width > -&& 8*(v * mb_y + y) < s->height) { > +if ( 8*(h * mb_x + x) < (s->width + (1 << > h_shift) - 1) >> h_shift > +&& 8*(v * mb_y + y) < (s->height + (1 << > v_shift) - 1) >> v_shift) { > >>> > >>> please move the w/h computation out of the block loop > >>> it stays the same for a component and does not need to be > >>> recalculated > >>> theres a loop above that fills data[] that can probably be used to > >>> fill w/h arrays > >> > >> OK, but since there are only two possible values, I don't think filling > >> arrays is necessary. Attached is an updated patch. > > > > couldnt there be a alpha plane too ? > > Yes, fixed patch attached. > I also tested filling w/h arrays, but that turned out to be slightly slower. > > Best regards, > Andreas > > mjpegdec.c | 11 --- > 1 file changed, 8 insertions(+), 3 deletions(-) > a294ce9a780fdd710d3661bc201b0c72d30786d3 > 0001-mjpegdec-consider-chroma-subsampling-in-size-check.patch > From 7788195340e1d0e1206660f12f003f952da750a6 Mon Sep 17 00:00:00 2001 > From: Andreas Cadhalpun > Date: Wed, 2 Dec 2015 21:52:23 +0100 > Subject: [PATCH] mjpegdec: consider chroma subsampling in size check > > If the chroma components are subsampled, smaller buffers are allocated > for them. In that case the maximal block_offset for the chroma > components is not as large as for the luma component. > > This fixes out of bounds writes causing segmentation faults or memory > corruption. > LGTM thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Many that live deserve death. And some that die deserve life. Can you give it to them? Then do not be too eager to deal out death in judgement. For even the very wise cannot see all ends. -- Gandalf signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mjpegdec: consider chroma subsampling in size check
On 05.12.2015 04:02, Michael Niedermayer wrote: > On Fri, Dec 04, 2015 at 03:14:21PM +0100, Andreas Cadhalpun wrote: >> On 03.12.2015 15:48, Michael Niedermayer wrote: >>> On Wed, Dec 02, 2015 at 10:00:13PM +0100, Andreas Cadhalpun wrote: @@ -1293,14 +1296,16 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int nb_components, int Ah, v = s->v_scount[i]; x = 0; y = 0; +h_shift = c ? chroma_h_shift: 0; +v_shift = c ? chroma_v_shift: 0; for (j = 0; j < n; j++) { block_offset = (((linesize[c] * (v * mb_y + y) * 8) + (h * mb_x + x) * 8 * bytes_per_pixel) >> s->avctx->lowres); if (s->interlaced && s->bottom_field) block_offset += linesize[c] >> 1; -if ( 8*(h * mb_x + x) < s->width -&& 8*(v * mb_y + y) < s->height) { +if ( 8*(h * mb_x + x) < (s->width + (1 << h_shift) - 1) >> h_shift +&& 8*(v * mb_y + y) < (s->height + (1 << v_shift) - 1) >> v_shift) { >>> >>> please move the w/h computation out of the block loop >>> it stays the same for a component and does not need to be >>> recalculated >>> theres a loop above that fills data[] that can probably be used to >>> fill w/h arrays >> >> OK, but since there are only two possible values, I don't think filling >> arrays is necessary. Attached is an updated patch. > > couldnt there be a alpha plane too ? Yes, fixed patch attached. I also tested filling w/h arrays, but that turned out to be slightly slower. Best regards, Andreas >From 7788195340e1d0e1206660f12f003f952da750a6 Mon Sep 17 00:00:00 2001 From: Andreas Cadhalpun Date: Wed, 2 Dec 2015 21:52:23 +0100 Subject: [PATCH] mjpegdec: consider chroma subsampling in size check If the chroma components are subsampled, smaller buffers are allocated for them. In that case the maximal block_offset for the chroma components is not as large as for the luma component. This fixes out of bounds writes causing segmentation faults or memory corruption. Signed-off-by: Andreas Cadhalpun --- libavcodec/mjpegdec.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c index 4c9c82d..c812b86 100644 --- a/libavcodec/mjpegdec.c +++ b/libavcodec/mjpegdec.c @@ -1246,7 +1246,7 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int nb_components, int Ah, int mb_bitmask_size, const AVFrame *reference) { -int i, mb_x, mb_y; +int i, mb_x, mb_y, chroma_h_shift, chroma_v_shift, chroma_width, chroma_height; uint8_t *data[MAX_COMPONENTS]; const uint8_t *reference_data[MAX_COMPONENTS]; int linesize[MAX_COMPONENTS]; @@ -1263,6 +1263,11 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int nb_components, int Ah, s->restart_count = 0; +av_pix_fmt_get_chroma_sub_sample(s->avctx->pix_fmt, &chroma_h_shift, + &chroma_v_shift); +chroma_width = FF_CEIL_RSHIFT(s->width, chroma_h_shift); +chroma_height = FF_CEIL_RSHIFT(s->height, chroma_v_shift); + for (i = 0; i < nb_components; i++) { int c = s->comp_index[i]; data[c] = s->picture_ptr->data[c]; @@ -1299,8 +1304,8 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int nb_components, int Ah, if (s->interlaced && s->bottom_field) block_offset += linesize[c] >> 1; -if ( 8*(h * mb_x + x) < s->width -&& 8*(v * mb_y + y) < s->height) { +if ( 8*(h * mb_x + x) < ((c == 1) || (c == 2) ? chroma_width : s->width) +&& 8*(v * mb_y + y) < ((c == 1) || (c == 2) ? chroma_height : s->height)) { ptr = data[c] + block_offset; } else ptr = NULL; -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mjpegdec: consider chroma subsampling in size check
On Fri, Dec 04, 2015 at 03:14:21PM +0100, Andreas Cadhalpun wrote: > On 03.12.2015 15:48, Michael Niedermayer wrote: > > On Wed, Dec 02, 2015 at 10:00:13PM +0100, Andreas Cadhalpun wrote: > >> @@ -1293,14 +1296,16 @@ static int mjpeg_decode_scan(MJpegDecodeContext > >> *s, int nb_components, int Ah, > >> v = s->v_scount[i]; > >> x = 0; > >> y = 0; > >> +h_shift = c ? chroma_h_shift: 0; > >> +v_shift = c ? chroma_v_shift: 0; > >> for (j = 0; j < n; j++) { > >> block_offset = (((linesize[c] * (v * mb_y + y) * 8) + > >> (h * mb_x + x) * 8 * > >> bytes_per_pixel) >> s->avctx->lowres); > >> > >> if (s->interlaced && s->bottom_field) > >> block_offset += linesize[c] >> 1; > >> -if ( 8*(h * mb_x + x) < s->width > >> -&& 8*(v * mb_y + y) < s->height) { > >> +if ( 8*(h * mb_x + x) < (s->width + (1 << h_shift) > >> - 1) >> h_shift > >> +&& 8*(v * mb_y + y) < (s->height + (1 << v_shift) > >> - 1) >> v_shift) { > > > > please move the w/h computation out of the block loop > > it stays the same for a component and does not need to be > > recalculated > > theres a loop above that fills data[] that can probably be used to > > fill w/h arrays > > OK, but since there are only two possible values, I don't think filling > arrays is necessary. Attached is an updated patch. couldnt there be a alpha plane too ? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Into a blind darkness they enter who follow after the Ignorance, they as if into a greater darkness enter who devote themselves to the Knowledge alone. -- Isha Upanishad signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mjpegdec: consider chroma subsampling in size check
On 03.12.2015 15:48, Michael Niedermayer wrote: > On Wed, Dec 02, 2015 at 10:00:13PM +0100, Andreas Cadhalpun wrote: >> @@ -1293,14 +1296,16 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, >> int nb_components, int Ah, >> v = s->v_scount[i]; >> x = 0; >> y = 0; >> +h_shift = c ? chroma_h_shift: 0; >> +v_shift = c ? chroma_v_shift: 0; >> for (j = 0; j < n; j++) { >> block_offset = (((linesize[c] * (v * mb_y + y) * 8) + >> (h * mb_x + x) * 8 * bytes_per_pixel) >> >> s->avctx->lowres); >> >> if (s->interlaced && s->bottom_field) >> block_offset += linesize[c] >> 1; >> -if ( 8*(h * mb_x + x) < s->width >> -&& 8*(v * mb_y + y) < s->height) { >> +if ( 8*(h * mb_x + x) < (s->width + (1 << h_shift) - >> 1) >> h_shift >> +&& 8*(v * mb_y + y) < (s->height + (1 << v_shift) - >> 1) >> v_shift) { > > please move the w/h computation out of the block loop > it stays the same for a component and does not need to be > recalculated > theres a loop above that fills data[] that can probably be used to > fill w/h arrays OK, but since there are only two possible values, I don't think filling arrays is necessary. Attached is an updated patch. > also is this not also needed in mjpeg_decode_scan_progressive_ac() ? I don't think so. While mjpeg_decode_scan calculates the y offset with '(v * mb_y + y) * 8', which can be larger than chroma_height (and even s->height), mjpeg_decode_scan_progressive_ac uses for some reason just 'mb_y * 8', which can't be larger than chroma_height, as far as I can tell. Best regards, Andreas >From 153494b62cf3e143a90eb8f02fdef5e017163f0b Mon Sep 17 00:00:00 2001 From: Andreas Cadhalpun Date: Wed, 2 Dec 2015 21:52:23 +0100 Subject: [PATCH] mjpegdec: consider chroma subsampling in size check If the chroma components are subsampled, smaller buffers are allocated for them. In that case the maximal block_offset for the chroma components is not as large as for the luma component. This fixes out of bounds writes causing segmentation faults or memory corruption. Signed-off-by: Andreas Cadhalpun --- libavcodec/mjpegdec.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c index cfc59ac..8dfcae0 100644 --- a/libavcodec/mjpegdec.c +++ b/libavcodec/mjpegdec.c @@ -1246,7 +1246,7 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int nb_components, int Ah, int mb_bitmask_size, const AVFrame *reference) { -int i, mb_x, mb_y; +int i, mb_x, mb_y, chroma_h_shift, chroma_v_shift, chroma_width, chroma_height; uint8_t *data[MAX_COMPONENTS]; const uint8_t *reference_data[MAX_COMPONENTS]; int linesize[MAX_COMPONENTS]; @@ -1263,6 +1263,11 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int nb_components, int Ah, s->restart_count = 0; +av_pix_fmt_get_chroma_sub_sample(s->avctx->pix_fmt, &chroma_h_shift, + &chroma_v_shift); +chroma_width = FF_CEIL_RSHIFT(s->width, chroma_h_shift); +chroma_height = FF_CEIL_RSHIFT(s->height, chroma_v_shift); + for (i = 0; i < nb_components; i++) { int c = s->comp_index[i]; data[c] = s->picture_ptr->data[c]; @@ -1299,8 +1304,8 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int nb_components, int Ah, if (s->interlaced && s->bottom_field) block_offset += linesize[c] >> 1; -if ( 8*(h * mb_x + x) < s->width -&& 8*(v * mb_y + y) < s->height) { +if ( 8*(h * mb_x + x) < (c ? chroma_width : s->width) +&& 8*(v * mb_y + y) < (c ? chroma_height : s->height)) { ptr = data[c] + block_offset; } else ptr = NULL; -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mjpegdec: consider chroma subsampling in size check
On Wed, Dec 02, 2015 at 10:00:13PM +0100, Andreas Cadhalpun wrote: > If the chroma components are subsampled, smaller buffers are allocated > for them. In that case the maximal block_offset for the chroma > components is not as large as for the luma component. > > This fixes out of bounds writes causing segmentation faults or memory > corruption. > > Signed-off-by: Andreas Cadhalpun > --- > libavcodec/mjpegdec.c | 13 + > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c > index cfc59ac..303b990 100644 > --- a/libavcodec/mjpegdec.c > +++ b/libavcodec/mjpegdec.c > @@ -1246,7 +1246,7 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int > nb_components, int Ah, > int mb_bitmask_size, > const AVFrame *reference) > { > -int i, mb_x, mb_y; > +int i, mb_x, mb_y, chroma_h_shift, chroma_v_shift; > uint8_t *data[MAX_COMPONENTS]; > const uint8_t *reference_data[MAX_COMPONENTS]; > int linesize[MAX_COMPONENTS]; > @@ -1271,6 +1271,9 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int > nb_components, int Ah, > s->coefs_finished[c] |= 1; > } > > +av_pix_fmt_get_chroma_sub_sample(s->avctx->pix_fmt, &chroma_h_shift, > + &chroma_v_shift); > + > for (mb_y = 0; mb_y < s->mb_height; mb_y++) { > for (mb_x = 0; mb_x < s->mb_width; mb_x++) { > const int copy_mb = mb_bitmask && !get_bits1(&mb_bitmask_gb); > @@ -1285,7 +1288,7 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int > nb_components, int Ah, > } > for (i = 0; i < nb_components; i++) { > uint8_t *ptr; > -int n, h, v, x, y, c, j; > +int n, h, v, x, y, c, j, h_shift, v_shift; > int block_offset; > n = s->nb_blocks[i]; > c = s->comp_index[i]; > @@ -1293,14 +1296,16 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, > int nb_components, int Ah, > v = s->v_scount[i]; > x = 0; > y = 0; > +h_shift = c ? chroma_h_shift: 0; > +v_shift = c ? chroma_v_shift: 0; > for (j = 0; j < n; j++) { > block_offset = (((linesize[c] * (v * mb_y + y) * 8) + > (h * mb_x + x) * 8 * bytes_per_pixel) > >> s->avctx->lowres); > > if (s->interlaced && s->bottom_field) > block_offset += linesize[c] >> 1; > -if ( 8*(h * mb_x + x) < s->width > -&& 8*(v * mb_y + y) < s->height) { > +if ( 8*(h * mb_x + x) < (s->width + (1 << h_shift) - > 1) >> h_shift > +&& 8*(v * mb_y + y) < (s->height + (1 << v_shift) - > 1) >> v_shift) { please move the w/h computation out of the block loop it stays the same for a component and does not need to be recalculated theres a loop above that fills data[] that can probably be used to fill w/h arrays also is this not also needed in mjpeg_decode_scan_progressive_ac() ? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Good people do not need laws to tell them to act responsibly, while bad people will find a way around the laws. -- Plato signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] mjpegdec: consider chroma subsampling in size check
If the chroma components are subsampled, smaller buffers are allocated for them. In that case the maximal block_offset for the chroma components is not as large as for the luma component. This fixes out of bounds writes causing segmentation faults or memory corruption. Signed-off-by: Andreas Cadhalpun --- libavcodec/mjpegdec.c | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c index cfc59ac..303b990 100644 --- a/libavcodec/mjpegdec.c +++ b/libavcodec/mjpegdec.c @@ -1246,7 +1246,7 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int nb_components, int Ah, int mb_bitmask_size, const AVFrame *reference) { -int i, mb_x, mb_y; +int i, mb_x, mb_y, chroma_h_shift, chroma_v_shift; uint8_t *data[MAX_COMPONENTS]; const uint8_t *reference_data[MAX_COMPONENTS]; int linesize[MAX_COMPONENTS]; @@ -1271,6 +1271,9 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int nb_components, int Ah, s->coefs_finished[c] |= 1; } +av_pix_fmt_get_chroma_sub_sample(s->avctx->pix_fmt, &chroma_h_shift, + &chroma_v_shift); + for (mb_y = 0; mb_y < s->mb_height; mb_y++) { for (mb_x = 0; mb_x < s->mb_width; mb_x++) { const int copy_mb = mb_bitmask && !get_bits1(&mb_bitmask_gb); @@ -1285,7 +1288,7 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int nb_components, int Ah, } for (i = 0; i < nb_components; i++) { uint8_t *ptr; -int n, h, v, x, y, c, j; +int n, h, v, x, y, c, j, h_shift, v_shift; int block_offset; n = s->nb_blocks[i]; c = s->comp_index[i]; @@ -1293,14 +1296,16 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int nb_components, int Ah, v = s->v_scount[i]; x = 0; y = 0; +h_shift = c ? chroma_h_shift: 0; +v_shift = c ? chroma_v_shift: 0; for (j = 0; j < n; j++) { block_offset = (((linesize[c] * (v * mb_y + y) * 8) + (h * mb_x + x) * 8 * bytes_per_pixel) >> s->avctx->lowres); if (s->interlaced && s->bottom_field) block_offset += linesize[c] >> 1; -if ( 8*(h * mb_x + x) < s->width -&& 8*(v * mb_y + y) < s->height) { +if ( 8*(h * mb_x + x) < (s->width + (1 << h_shift) - 1) >> h_shift +&& 8*(v * mb_y + y) < (s->height + (1 << v_shift) - 1) >> v_shift) { ptr = data[c] + block_offset; } else ptr = NULL; -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel