[FFmpeg-devel] [PATCH] mjpegdec: consider chroma subsampling in size check

[Michael Niedermayer]

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 <Andreas.Cadhalpun at googlemail.com>
> ---
>  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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151203/d1094b1d/attachment.sig>