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

2015-12-06 Thread Andreas Cadhalpun 
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, _h_shift,
+ _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

2015-12-06 Thread Andreas Cadhalpun 
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

2015-12-06 Thread Michael Niedermayer 
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

2015-12-04 Thread Andreas Cadhalpun 
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, _h_shift,
+ _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

2015-12-04 Thread Michael Niedermayer 
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

2015-12-03 Thread 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 
> ---
>  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, _h_shift,
> + _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(_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

2015-12-02 Thread Andreas Cadhalpun 
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, _h_shift,
+ _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(_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