Re: [FFmpeg-devel] [PATCH] tiff: fix leaking yuv_line

[Andreas Cadhalpun]

On 16.02.2017 03:15, Michael Niedermayer wrote:
> On Thu, Feb 16, 2017 at 12:23:28AM +0100, Andreas Cadhalpun wrote:
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavcodec/tiff.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
>> index efbd9791a5..474ea90015 100644
>> --- a/libavcodec/tiff.c
>> +++ b/libavcodec/tiff.c
>> @@ -1386,6 +1386,7 @@ static av_cold int tiff_end(AVCodecContext *avctx)
>>  
>>  ff_lzw_decode_close(>lzw);
>>  av_freep(>deinvert_buf);
>> +av_freep(>yuv_line);
>>  return 0;
> 
> I assume we are missing a test in fate for the yuv case
> adding such test would be usefull

Indeed, I'll send a patch adding one.

> yuv_line_size should be reset to 0, not sure its ever needed but it
> feels more proper

It's in the close function, so it's unlikely to be needed, but I added
it now anyway. Updated patch is attached. 

Best regards,
Andreas
>From c9a5c531c1d0434c989998eab6cb1ac352200695 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
Date: Thu, 16 Feb 2017 00:07:24 +0100
Subject: [PATCH] tiff: fix leaking yuv_line

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavcodec/tiff.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
index efbd9791a5..650a9d89ef 100644
--- a/libavcodec/tiff.c
+++ b/libavcodec/tiff.c
@@ -1386,6 +1386,8 @@ static av_cold int tiff_end(AVCodecContext *avctx)
 
 ff_lzw_decode_close(>lzw);
 av_freep(>deinvert_buf);
+av_freep(>yuv_line);
+s->yuv_line_size = 0;
 return 0;
 }
 
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] mpegaudiodec_template: fix leaking fdsp for mp3on4float

[Andreas Cadhalpun]

On 16.02.2017 03:12, Michael Niedermayer wrote:
> On Thu, Feb 16, 2017 at 12:39:17AM +0100, Andreas Cadhalpun wrote:
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavcodec/mpegaudiodec_template.c | 3 +++
>>  1 file changed, 3 insertions(+)
> 
> should be ok

Pushed.

> why was this not detected by fate ?
> are we lacking a test for this ?

There is no fate test and since it uses float, it seems non-trivial to
add a platform-independent one.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] wmaprodec: fix leaking fdsp on init failure

[Andreas Cadhalpun]

On 16.02.2017 03:04, Michael Niedermayer wrote:
> On Thu, Feb 16, 2017 at 12:56:38AM +0100, Andreas Cadhalpun wrote:
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavcodec/wmaprodec.c | 7 ---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> should be ok

Pushed.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] bink: fix leaking last frame on error

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavcodec/bink.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libavcodec/bink.c b/libavcodec/bink.c
index cc55870114..91004a6ae5 100644
--- a/libavcodec/bink.c
+++ b/libavcodec/bink.c
@@ -1299,10 +1299,6 @@ static av_cold int decode_init(AVCodecContext *avctx)
 }
 c->avctx = avctx;
 
-c->last = av_frame_alloc();
-if (!c->last)
-return AVERROR(ENOMEM);
-
 if ((ret = av_image_check_size(avctx->width, avctx->height, 0, avctx)) < 0)
 return ret;
 
@@ -1317,6 +1313,10 @@ static av_cold int decode_init(AVCodecContext *avctx)
 return ret;
 }
 
+c->last = av_frame_alloc();
+if (!c->last)
+return AVERROR(ENOMEM);
+
 if (c->version == 'b') {
 if (!binkb_initialised) {
 binkb_calc_quant();
-- 
2.11.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] mpegaudiodec_template: fix leaking fdsp for mp3on4float

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavcodec/mpegaudiodec_template.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libavcodec/mpegaudiodec_template.c 
b/libavcodec/mpegaudiodec_template.c
index 1114428f33..53c09edced 100644
--- a/libavcodec/mpegaudiodec_template.c
+++ b/libavcodec/mpegaudiodec_template.c
@@ -1828,6 +1828,9 @@ static av_cold int decode_close_mp3on4(AVCodecContext * 
avctx)
 MP3On4DecodeContext *s = avctx->priv_data;
 int i;
 
+if (s->mp3decctx[0])
+av_freep(>mp3decctx[0]->fdsp);
+
 for (i = 0; i < s->frames; i++)
 av_freep(>mp3decctx[i]);
 
-- 
2.11.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] tiff: fix leaking yuv_line

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavcodec/tiff.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
index efbd9791a5..474ea90015 100644
--- a/libavcodec/tiff.c
+++ b/libavcodec/tiff.c
@@ -1386,6 +1386,7 @@ static av_cold int tiff_end(AVCodecContext *avctx)
 
 ff_lzw_decode_close(>lzw);
 av_freep(>deinvert_buf);
+av_freep(>yuv_line);
 return 0;
 }
 
-- 
2.11.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] speedhq: make sure the block index is not negative

[Andreas Cadhalpun]

On 01.02.2017 17:25, Steinar H. Gunderson wrote:
> On Wed, Feb 01, 2017 at 02:17:05AM +0100, Andreas Cadhalpun wrote:
>>> Would you mind sharing an input where this actually triggers? None of the
>>> samples I have seem to trigger this, so I suppose it's some sort of fuzzed
>>> input.
>> Indeed it is. I've sent you a sample.
> 
> Could you please try the attached patch?

It works fine, thanks! I've applied it.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 9/9] boadec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 31.01.2017 13:45, Ronald S. Bultje wrote:
> I don't want this patch. I also don't want to discuss it further. Please
> remove the log message. Thank you.

For the sake of ending this, I've removed them.
Please avoid complaining about log messages for my future patches. Thank you.

Best regards,
Andreas 
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] pgssubdec: reset rle_data_len/rle_remaining_len on allocation error

[Andreas Cadhalpun]

On 31.01.2017 15:13, Michael Niedermayer wrote:
> On Tue, Jan 31, 2017 at 01:59:38AM +0100, Andreas Cadhalpun wrote:
>> The code relies on their validity and otherwise can try to access a NULL
>> object->rle pointer, causing segmentation faults.
>>
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavcodec/pgssubdec.c | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> LGTM

Pushed.

> please also backport this to the releases

Will do.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] speedhq: make sure the block index is not negative

[Andreas Cadhalpun]

On 31.01.2017 09:43, Steinar H. Gunderson wrote:
> On Tue, Jan 31, 2017 at 01:57:31AM +0100, Andreas Cadhalpun wrote:
>>> This sounds like a strangeness in constructing the table, which shouldn't be
>>> papered over in the inner loop of the decoder.
>> Maybe, I don't know what the contents of the table should be, but the 
>> following
>> are {-1, 0}: 32, 33, 64, 96, 128
> 
> Seemingly they are, indeed.
> 
>>> Do you have an actual input where your code makes a difference?
>> Yes, without this patch ubsan reports:
>> src/libavcodec/speedhq.c:206:13: runtime error: index -1 out of bounds for 
>> type 'uint8_t [128]'
> 
> Would you mind sharing an input where this actually triggers? None of the
> samples I have seem to trigger this, so I suppose it's some sort of fuzzed
> input.

Indeed it is. I've sent you a sample.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 9/9] boadec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 30.01.2017 17:17, Ronald S. Bultje wrote:
> On Sun, Jan 29, 2017 at 7:37 PM, Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com> wrote:
> 
>> On 29.01.2017 04:46, Ronald S. Bultje wrote:
>>> Hm ... So I guess I wasn't clear about this, but the reason I didn't
>> reply
>>> to other patches with log messages was not because I'm OK with, but
>> simply
>>> to keep the discussion contained in a single thread and not spam the
>> list.
>>> I'd prefer if the log msg disappeared from all fuzz-only checks...
>>
>> Being a "fuzz-only check" is not a well-defined concept. Anything a fuzzer
>> does could in principle also happen due to file corruption.
> 
> 
> Please stop already. This is ridiculous.

Please don't ignore the rest of my argument nor laugh at it.

> Error messages are supposed to help solve a problem. A corrupt file isn't
> solvable.

It can be solved by restoring a backup, for example.

> No error message can help. So why bother informing the user?

To give the user some clue what's wrong. It's impossible to tell
in advance, whether or not that will actually help him.

> "Channel count too large" is the same as "fluffybuzz whackabit
> mackahooloo".

It obviously isn't: Unlike the latter, the former can be understood
by most English speaking persons.

Also the log message added here was "Too many channels", which already
had 9 occurrences in the code base and which is quite similar to
"Too many invisible frames", which you added.
It is completely arbitrary that you bikeshed my patches, while you
apparently have no problem with those.

> There is no action associated with the message that can help
> solve it. This is different from "encryption key missing, please use the
> option -key value to specify" for encrypted content.

Almost no log message in the ffmpeg code base contains a direct action.
Do you want to remove all other log messages?

> All we're doing is growing source and binary size and code complexity, and
> we do so without any apparent benefit. I just don't think that's a great
> approach.

Marton proposed a way to mitigate this [1], but you haven't commented
on it so far.

Best regards,
Andreas


1: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206327.html
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 4/9] ircamdec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 30.01.2017 09:52, Paul B Mahol wrote:
> On 1/30/17, Andreas Cadhalpun <andreas.cadhal...@googlemail.com> wrote:
>> On 29.01.2017 02:34, Michael Niedermayer wrote:
>>> On Thu, Jan 26, 2017 at 02:12:19AM +0100, Andreas Cadhalpun wrote:
>>>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>>>> ---
>>>>  libavformat/ircamdec.c | 6 ++
>>>>  1 file changed, 6 insertions(+)
>>>
>>> LGTM assuming the author/maintainer does not object, maybe he
>>> prefers this without the log message
>>
>> Attached is a variant without the log message.
>>
> 
> ok

Pushed.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] pgssubdec: reset rle_data_len/rle_remaining_len on allocation error

[Andreas Cadhalpun]

The code relies on their validity and otherwise can try to access a NULL
object->rle pointer, causing segmentation faults.

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavcodec/pgssubdec.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavcodec/pgssubdec.c b/libavcodec/pgssubdec.c
index b50b37b206..b897d72aab 100644
--- a/libavcodec/pgssubdec.c
+++ b/libavcodec/pgssubdec.c
@@ -300,8 +300,11 @@ static int parse_object_segment(AVCodecContext *avctx,
 
 av_fast_padded_malloc(>rle, >rle_buffer_size, 
rle_bitmap_len);
 
-if (!object->rle)
+if (!object->rle) {
+object->rle_data_len = 0;
+object->rle_remaining_len = 0;
 return AVERROR(ENOMEM);
+}
 
 memcpy(object->rle, buf, buf_size);
 object->rle_data_len = buf_size;
-- 
2.11.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] speedhq: make sure the block index is not negative

[Andreas Cadhalpun]

On 31.01.2017 00:59, Steinar H. Gunderson wrote:
> On Tue, Jan 31, 2017 at 12:49:56AM +0100, Andreas Cadhalpun wrote:
>>> How can you get a negative run, which would be required for this to happen?
>> Some values in ff_dc_alpha_run_vlc_le.table are negative, e.g.:
>>  ff_dc_alpha_run_vlc_le.table[32] = {-1, 0}
> 
> This sounds like a strangeness in constructing the table, which shouldn't be
> papered over in the inner loop of the decoder.

Maybe, I don't know what the contents of the table should be, but the following
are {-1, 0}: 32, 33, 64, 96, 128

> I might be missing something
> in how this table is used internally in the bitstream reader, but a code of
> 32 should just immediately hit 0 in the LSB and be interpreted as run=0.
> (There are no invalid codes in this VLC, so nothing like run=-1 should ever
> happen.)
> 
> Do you have an actual input where your code makes a difference?

Yes, without this patch ubsan reports:
src/libavcodec/speedhq.c:206:13: runtime error: index -1 out of bounds for type 
'uint8_t [128]'

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] speedhq: make sure the block index is not negative

[Andreas Cadhalpun]

On 30.01.2017 09:23, Steinar H. Gunderson wrote:
> How can you get a negative run, which would be required for this to happen?

Some values in ff_dc_alpha_run_vlc_le.table are negative, e.g.:
 ff_dc_alpha_run_vlc_le.table[32] = {-1, 0}

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] speedhq: make sure the block index is not negative

[Andreas Cadhalpun]

Fixes out-of-bounds writes.

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavcodec/speedhq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/speedhq.c b/libavcodec/speedhq.c
index 385f779f83..6ae1e0f8df 100644
--- a/libavcodec/speedhq.c
+++ b/libavcodec/speedhq.c
@@ -198,7 +198,7 @@ static inline int decode_alpha_block(const SHQContext *s, 
GetBitContext *gb, uin
 
 if (run == 128) break;
 i += run;
-if (i >= 128)
+if (i < 0 || i >= 128)
 return AVERROR_INVALIDDATA;
 
 UPDATE_CACHE_LE(re, gb);
-- 
2.11.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 8/9] xvag: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 29.01.2017 09:51, Paul B Mahol wrote:
> On 1/29/17, Michael Niedermayer <michae...@gmx.at> wrote:
>> On Thu, Jan 26, 2017 at 02:13:48AM +0100, Andreas Cadhalpun wrote:
>>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>>> ---
>>>  libavformat/xvag.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> LGTM assuming the author/maintainer does not object
> 
> ok

Pushed.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/9] genh: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 29.01.2017 09:52, Paul B Mahol wrote:
> On 1/29/17, Michael Niedermayer <michae...@gmx.at> wrote:
>> On Thu, Jan 26, 2017 at 02:11:54AM +0100, Andreas Cadhalpun wrote:
>>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>>> ---
>>>  libavformat/genh.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> LGTM assuming the author/maintainer does not object
>>
> 
> ok

Pushed.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 7/9] epafdec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 29.01.2017 09:51, Paul B Mahol wrote:
> On 1/29/17, Michael Niedermayer <michae...@gmx.at> wrote:
>> On Thu, Jan 26, 2017 at 02:13:33AM +0100, Andreas Cadhalpun wrote:
>>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>>> ---
>>>  libavformat/epafdec.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> LGTM assuming the author/maintainer does not object
> 
> ok

Pushed.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 4/9] ircamdec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 29.01.2017 02:34, Michael Niedermayer wrote:
> On Thu, Jan 26, 2017 at 02:12:19AM +0100, Andreas Cadhalpun wrote:
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavformat/ircamdec.c | 6 ++
>>  1 file changed, 6 insertions(+)
> 
> LGTM assuming the author/maintainer does not object, maybe he
> prefers this without the log message

Attached is a variant without the log message.

Best regards,
Andreas

>From bf03bedf16ee4659defdca1b82eb213448d00f59 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
Date: Thu, 15 Dec 2016 02:14:45 +0100
Subject: [PATCH] ircamdec: prevent overflow during block alignment calculation

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/ircamdec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libavformat/ircamdec.c b/libavformat/ircamdec.c
index 59f3a49411..a6b7a280f3 100644
--- a/libavformat/ircamdec.c
+++ b/libavformat/ircamdec.c
@@ -20,6 +20,7 @@
  */
 
 #include "libavutil/intreadwrite.h"
+#include "libavcodec/internal.h"
 #include "avformat.h"
 #include "internal.h"
 #include "pcm.h"
@@ -87,6 +88,8 @@ static int ircam_read_header(AVFormatContext *s)
 
 st->codecpar->codec_type  = AVMEDIA_TYPE_AUDIO;
 st->codecpar->channels= channels;
+if (st->codecpar->channels > FF_SANE_NB_CHANNELS)
+return AVERROR(ENOSYS);
 st->codecpar->sample_rate = sample_rate;
 
 st->codecpar->codec_id = ff_codec_get_id(tags, tag);
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 5/9] nistspheredec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 29.01.2017 04:02, Marton Balint wrote:
> 
> On Sun, 29 Jan 2017, Andreas Cadhalpun wrote:
> 
>> On 28.01.2017 12:44, Marton Balint wrote:
>>> If we reduce the number of extra lines (not at any cost), I think that 
>>> helps.
>>> There is also a solution which keeps the traditional C syntax, and is easy 
>>> to undestand even at first glance.
>>>
>>> if (st->codecpar->channels > FF_SANE_NB_CHANNELS)
>>> return ff_elog(AVERROR(ENOSYS), s, "Too many channels %d > %d\n", 
>>> st->codecpar->channels, FF_SANE_NB_CHANNELS);
>>
>> How would you define ff_elog for this to work?
>>
> 
> static inline int ff_elog(int error, void *log_ctx, const char *fmt, ...) {
> if (!CONFIG_SMALL) {
> va_list vl;
> va_start(vl, fmt);
> av_vlog(log_ctx, AV_LOG_ERROR, fmt, vl);
> va_end(vl);
> }
> return error;
> }

OK. I'd be fine with using it, if people prefer this way.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 9/9] boadec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 29.01.2017 04:46, Ronald S. Bultje wrote:
> Hm ... So I guess I wasn't clear about this, but the reason I didn't reply
> to other patches with log messages was not because I'm OK with, but simply
> to keep the discussion contained in a single thread and not spam the list.
> I'd prefer if the log msg disappeared from all fuzz-only checks...

Being a "fuzz-only check" is not a well-defined concept. Anything a fuzzer
does could in principle also happen due to file corruption.
For header parsing such errors could also happen if a file gets misdetected
and thus a wrong demuxer is used.

So what do you mean with "fuzz-only check"?
For example, would you consider the error check I quoted in the other
thread [1] a "fuzz-only check"?

It's clear that you prefer fewer log messages than I do, but in the absence
of a general consensus about this topic, every author/maintainer can decide
which log messages are wanted in their own code.

Best regards,
Andreas


1: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206312.html
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/9] electronicarts: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 27.01.2017 01:44, Michael Niedermayer wrote:
> On Thu, Jan 26, 2017 at 02:11:31AM +0100, Andreas Cadhalpun wrote:
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavformat/electronicarts.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> LGTM

Pushed.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 9/9] boadec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 27.01.2017 03:27, Michael Niedermayer wrote:
> On Thu, Jan 26, 2017 at 02:14:09AM +0100, Andreas Cadhalpun wrote:
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavformat/boadec.c | 14 +-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> LGTM

Pushed.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 6/9] pvfdec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 26.01.2017 09:37, Paul B Mahol wrote:
> On 1/26/17, Andreas Cadhalpun <andreas.cadhal...@googlemail.com> wrote:
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavformat/pvfdec.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
> 
> lgtm

Pushed.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/9] 4xm: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 26.01.2017 03:21, Michael Niedermayer wrote:
> On Thu, Jan 26, 2017 at 02:11:02AM +0100, Andreas Cadhalpun wrote:
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavformat/4xm.c | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> LGTM
> 
> thx

Pushed.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 5/9] nistspheredec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 29.01.2017 00:26, Paul B Mahol wrote:
> On 1/29/17, Andreas Cadhalpun <andreas.cadhal...@googlemail.com> wrote:
>> On 28.01.2017 12:44, Marton Balint wrote:
>>> If we reduce the number of extra lines (not at any cost), I think that
>>> helps.
>>> There is also a solution which keeps the traditional C syntax, and is easy
>>> to undestand even at first glance.
>>>
>>> if (st->codecpar->channels > FF_SANE_NB_CHANNELS)
>>> return ff_elog(AVERROR(ENOSYS), s, "Too many channels %d > %d\n",
>>> st->codecpar->channels, FF_SANE_NB_CHANNELS);
>>
>> How would you define ff_elog for this to work?
> 
> I'm maintainer of this file, and I'm fed up with this nuisance conversation.
> I'm against log message.

Fair enough, attached is a patch without the log messages in this file.

However, this discussion is also about error logging in general and it
should come to some conclusion that prevents this nuisance from recurring
for future patches.

Best regards,
Andreas
>From 2386e24e38bbf9847870dfec22998e8fa252e359 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
Date: Thu, 15 Dec 2016 02:14:49 +0100
Subject: [PATCH] nistspheredec: prevent overflow during block alignment
 calculation

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/nistspheredec.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c
index 782d1dfbfb..588174482c 100644
--- a/libavformat/nistspheredec.c
+++ b/libavformat/nistspheredec.c
@@ -21,6 +21,7 @@
 
 #include "libavutil/avstring.h"
 #include "libavutil/intreadwrite.h"
+#include "libavcodec/internal.h"
 #include "avformat.h"
 #include "internal.h"
 #include "pcm.h"
@@ -90,6 +91,8 @@ static int nist_read_header(AVFormatContext *s)
 return 0;
 } else if (!memcmp(buffer, "channel_count", 13)) {
 sscanf(buffer, "%*s %*s %"SCNd32, >codecpar->channels);
+if (st->codecpar->channels > FF_SANE_NB_CHANNELS)
+return AVERROR(ENOSYS);
 } else if (!memcmp(buffer, "sample_byte_format", 18)) {
 sscanf(buffer, "%*s %*s %31s", format);
 
@@ -109,6 +112,8 @@ static int nist_read_header(AVFormatContext *s)
 sscanf(buffer, "%*s %*s %"SCNd64, >duration);
 } else if (!memcmp(buffer, "sample_n_bytes", 14)) {
 sscanf(buffer, "%*s %*s %"SCNd32, );
+if (bps > (INT_MAX / FF_SANE_NB_CHANNELS) >> 3)
+return AVERROR_INVALIDDATA;
 } else if (!memcmp(buffer, "sample_rate", 11)) {
 sscanf(buffer, "%*s %*s %"SCNd32, >codecpar->sample_rate);
 } else if (!memcmp(buffer, "sample_sig_bits", 15)) {
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 5/9] nistspheredec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 28.01.2017 12:44, Marton Balint wrote:
> If we reduce the number of extra lines (not at any cost), I think that helps.
> There is also a solution which keeps the traditional C syntax, and is easy to 
> undestand even at first glance.
> 
> if (st->codecpar->channels > FF_SANE_NB_CHANNELS)
> return ff_elog(AVERROR(ENOSYS), s, "Too many channels %d > %d\n", 
> st->codecpar->channels, FF_SANE_NB_CHANNELS);

How would you define ff_elog for this to work?

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 5/9] nistspheredec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 28.01.2017 03:48, Ronald S. Bultje wrote:
> I agree a macro here doesn't help. My concern wasn't with the check itself,
> I agree a file with 100 channels should error out. My concern is that these
> files will universally be the result of fuzzing, so I don't want to spam
> stderr with messages related to it, nor do I want source/binary size to
> increase because of it.
> 
> If you make ff_elog similar to assert (only if NDEBUG is not set), that may
> work for the binary size concern, but the source code size is still a
> concern. Again, not because it's bad code, but because it's needless since
> it only happens for fuzzed samples.

You claim that, but it's impossible to prove and thus likely wrong.

Also it's quite arbitrary that you object to this log message, while e.g.
the following has been there for years:
 if (s->nb_streams == ASF_MAX_STREAMS) {
 av_log(s, AV_LOG_ERROR, "too many streams\n");
 return AVERROR(EINVAL);
 }

Unless you can come up with objective criteria, when to add log messages
and when not, this topic is going to be a pointless waste of time.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 5/9] nistspheredec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 27.01.2017 02:56, Marton Balint wrote:
> I see 3 problems (wm4 explicitly named them, but I also had them in mind)
> - Little benefit, yet
> - Makes the code less clean, more cluttered
> - Increases binary size
> 
> The ideas I proposed (use macros, use common / factorized checks for common
> validatons and errors) might be a good compromise IMHO. Fuzzing thereforce
> can be done with "debug" builds.
> 
> Anyway, I am not blocking the patch, just expressing what I would prefer in
> the long run.

How about the following macro?
#define FF_RETURN_ERROR(condition, error, log_ctx, ...) {   \
if (condition) {\
if (!CONFIG_SMALL && log_ctx)   \
av_log(log_ctx, AV_LOG_ERROR, __VA_ARGS__); \
return error;   \
}   \
}

That could be used with error message:
FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, 
AVERROR(ENOSYS),
s, "Too many channels %d > %d\n", st->codecpar->channels, 
FF_SANE_NB_CHANNELS)

Or without:
FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, 
AVERROR(ENOSYS), NULL)

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] mpeg12dec: validate color space

[Andreas Cadhalpun]

On 26.01.2017 13:59, Ronald S. Bultje wrote:
> On Wed, Jan 25, 2017 at 8:56 PM, Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com> wrote:
>> On 26.01.2017 02:26, Ronald S. Bultje wrote:
>>> The question is not whether changing A or B towards the other makes the
>>> combination consistent. The question is which form of consistency is
>>> better...
>>
>> As new values don't get added that often, I don't see a problem with
>> requiring
>> to explicitly add them to libavutil before being able to use them.
> 
> 
> That is because your use case for libavcodec is constrained, you use it
> only for decoding videos and fuzzing.

Please refrain from making such presumptuous, false claims in the future.

> There are others in this community
> that do a lot more than that with libavcodec.
> 
> Look, Andreas, I appreciate the work you're doing, I really do, but you
> always pick a fight with every review I put out on your code.

That's a grossly distorted picture of reality [1][2][3].

> I don't
> understand why. My reviews are not difficult to address, they really are
> not. My reviews are actionable, and if the action is taken, I'm happy and
> the code can be committed.

I appreciate your reviews, as long as they remain technical.

> Why pick a fight? What is the point? Do you
> think I'm an idiot that has no clue what he's doing and should be shot down
> because of that?

Such polemic phrases are inappropriate in any technical discussion.

> Please appreciate that I do have some clue of what I'm
> doing, and I am looking out for the health of the project, just like you,
> but with a slightly different perspective to some things. If I'm wrong,
> please point it out, I make mistakes also, but in cases like these, it can
> also help to just drop it and move on. Resolving an issue is not losing, it
> is winning.

Technical issues should be resolved with convincing arguments and not by
asking anyone who disagrees with you to move on.

For example, you could explain why you think that allowing unknown values
here is important, even though new ones are added only rarely.

Best regards,
Andreas


1: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-January/205192.html
2: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-January/205406.html
3: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-January/205414.html

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 5/9] nistspheredec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 26.01.2017 02:29, Ronald S. Bultje wrote:
> On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com> wrote:
> 
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavformat/nistspheredec.c | 11 +++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c
>> index 782d1dfbfb..3386497682 100644
>> --- a/libavformat/nistspheredec.c
>> +++ b/libavformat/nistspheredec.c
>> @@ -21,6 +21,7 @@
>>
>>  #include "libavutil/avstring.h"
>>  #include "libavutil/intreadwrite.h"
>> +#include "libavcodec/internal.h"
>>  #include "avformat.h"
>>  #include "internal.h"
>>  #include "pcm.h"
>> @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s)
>>  return 0;
>>  } else if (!memcmp(buffer, "channel_count", 13)) {
>>  sscanf(buffer, "%*s %*s %"SCNd32, >codecpar->channels);
>> +if (st->codecpar->channels > FF_SANE_NB_CHANNELS) {
>> +av_log(s, AV_LOG_ERROR, "Too many channels %d > %d\n",
>> +   st->codecpar->channels, FF_SANE_NB_CHANNELS);
>> +return AVERROR(ENOSYS);
>> +}
> 
> 
> I've said this before, but again - please don't add useless log messages.

I disagree that these log messages are useless and I'm not the only one [1].

> These don't help end users at all, since these samples are exceedingly
> unlikely to be real.

Files can get corrupted randomly, so I think this error isn't less likely
than most other errors.

> Most likely, they derive from fuzzing, and stderr
> cramming is one of the things that makes fuzzing slower.

Printing log messages in inner decoding loops makes that slower, but a
log message during header parsing is hardly noticeable.

Best regards,
Andreas


1: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-January/205433.html

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] mpeg12dec: validate color space

[Andreas Cadhalpun]

On 26.01.2017 02:26, Ronald S. Bultje wrote:
> Hi,
> 
> On Wed, Jan 25, 2017 at 8:20 PM, Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com> wrote:
> 
>> On 07.01.2017 13:44, Ronald S. Bultje wrote:
>>> Doesn't this destroy exporting of newly added colorspaces that have no
>>> explicit mention yet in libavutil? I'm not 100% sure this is a good
>> thing.
>>
>> Yes. That's how it is already done in libavcodec/h264_ps.c, so it just
>> makes handling unknown values consistent.
> 
> 
> Changing h264_ps.c would also make things consistent.

No, because also libavcodec/options_table.h limits the colorspace to known 
values.

> The question is not whether changing A or B towards the other makes the
> combination consistent. The question is which form of consistency is
> better...

As new values don't get added that often, I don't see a problem with requiring
to explicitly add them to libavutil before being able to use them.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] mpeg12dec: validate color space

[Andreas Cadhalpun]

On 07.01.2017 13:44, Ronald S. Bultje wrote:
> On Fri, Jan 6, 2017 at 9:35 PM, Michael Niedermayer <mich...@niedermayer.cc>
> wrote:
> 
>> On Fri, Jan 06, 2017 at 09:43:24PM +0100, Andreas Cadhalpun wrote:
>>> On 23.12.2016 00:57, Andreas Cadhalpun wrote:
>>>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>>>> ---
>>>>  libavcodec/mpeg12dec.c | 4 
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
>>>> index 63979079c8..d3dc67ad6a 100644
>>>> --- a/libavcodec/mpeg12dec.c
>>>> +++ b/libavcodec/mpeg12dec.c
>>>> @@ -1470,6 +1470,10 @@ static void 
>>>> mpeg_decode_sequence_display_extension(Mpeg1Context
>> *s1)
>>>>  s->avctx->color_primaries = get_bits(>gb, 8);
>>>>  s->avctx->color_trc   = get_bits(>gb, 8);
>>>>  s->avctx->colorspace  = get_bits(>gb, 8);
>>>> +if (!av_color_space_name(s->avctx->colorspace)) {
>>>> +av_log(s->avctx, AV_LOG_WARNING, "Invalid color space %d,
>> setting to unspecified\n", s->avctx->colorspace);
>>>> +s->avctx->colorspace = AVCOL_SPC_UNSPECIFIED;
>>>> +}
>>>>  }
>>>>  w = get_bits(>gb, 14);
>>>>  skip_bits(>gb, 1); // marker
>>>>
>>>
>>> Ping for the series.
>>
>> i have no real objection to it.
>> iam used to see these being exported unchanged though so it feels a
>> bit odd
> 
> 
> Doesn't this destroy exporting of newly added colorspaces that have no
> explicit mention yet in libavutil? I'm not 100% sure this is a good thing.

Yes. That's how it is already done in libavcodec/h264_ps.c, so it just
makes handling unknown values consistent.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 9/9] boadec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/boadec.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/libavformat/boadec.c b/libavformat/boadec.c
index ac2a33b3f0..6055effcad 100644
--- a/libavformat/boadec.c
+++ b/libavformat/boadec.c
@@ -20,6 +20,7 @@
  */
 
 #include "libavutil/intreadwrite.h"
+#include "libavcodec/internal.h"
 #include "avformat.h"
 #include "internal.h"
 
@@ -53,9 +54,20 @@ static int read_header(AVFormatContext *s)
 avio_rl32(s->pb);
 st->codecpar->sample_rate = avio_rl32(s->pb);
 st->codecpar->channels= avio_rl32(s->pb);
+if (st->codecpar->channels > FF_SANE_NB_CHANNELS) {
+av_log(s, AV_LOG_ERROR, "Too many channels %d > %d\n",
+   st->codecpar->channels, FF_SANE_NB_CHANNELS);
+return AVERROR(ENOSYS);
+}
 s->internal->data_offset = avio_rl32(s->pb);
 avio_r8(s->pb);
-st->codecpar->block_align = st->codecpar->channels * avio_rl32(s->pb);
+st->codecpar->block_align = avio_rl32(s->pb);
+if (st->codecpar->block_align > INT_MAX / FF_SANE_NB_CHANNELS) {
+av_log(s, AV_LOG_ERROR, "Too large block alignment %d > %d\n",
+   st->codecpar->block_align, INT_MAX / FF_SANE_NB_CHANNELS);
+return AVERROR_INVALIDDATA;
+}
+st->codecpar->block_align *= st->codecpar->channels;
 
 avio_seek(s->pb, s->internal->data_offset, SEEK_SET);
 
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 8/9] xvag: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/xvag.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libavformat/xvag.c b/libavformat/xvag.c
index 5ef4fb0900..22e4f1e3c8 100644
--- a/libavformat/xvag.c
+++ b/libavformat/xvag.c
@@ -20,6 +20,7 @@
  */
 
 #include "libavutil/bswap.h"
+#include "libavcodec/internal.h"
 #include "avformat.h"
 #include "internal.h"
 
@@ -68,7 +69,7 @@ static int xvag_read_header(AVFormatContext *s)
 
 if (st->codecpar->sample_rate <= 0)
 return AVERROR_INVALIDDATA;
-if (st->codecpar->channels <= 0)
+if (st->codecpar->channels <= 0 || st->codecpar->channels > 
FF_SANE_NB_CHANNELS)
 return AVERROR_INVALIDDATA;
 
 switch (codec) {
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 7/9] epafdec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/epafdec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libavformat/epafdec.c b/libavformat/epafdec.c
index 29190fff72..0cd9627a4b 100644
--- a/libavformat/epafdec.c
+++ b/libavformat/epafdec.c
@@ -20,6 +20,7 @@
  */
 
 #include "libavutil/intreadwrite.h"
+#include "libavcodec/internal.h"
 #include "avformat.h"
 #include "internal.h"
 #include "pcm.h"
@@ -59,7 +60,7 @@ static int epaf_read_header(AVFormatContext *s)
 channels= avio_rb32(s->pb);
 }
 
-if (!channels || !sample_rate)
+if (channels <= 0 || channels > FF_SANE_NB_CHANNELS || sample_rate <= 0)
 return AVERROR_INVALIDDATA;
 
 st = avformat_new_stream(s, NULL);
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 6/9] pvfdec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/pvfdec.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavformat/pvfdec.c b/libavformat/pvfdec.c
index b9f6d4f2c2..c6652b9b43 100644
--- a/libavformat/pvfdec.c
+++ b/libavformat/pvfdec.c
@@ -19,6 +19,7 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
+#include "libavcodec/internal.h"
 #include "avformat.h"
 #include "internal.h"
 #include "pcm.h"
@@ -44,7 +45,8 @@ static int pvf_read_header(AVFormatContext *s)
) != 3)
 return AVERROR_INVALIDDATA;
 
-if (channels <= 0 || bps <= 0 || sample_rate <= 0)
+if (channels <= 0 || channels > FF_SANE_NB_CHANNELS ||
+bps <= 0 || bps > INT_MAX / FF_SANE_NB_CHANNELS || sample_rate <= 0)
 return AVERROR_INVALIDDATA;
 
 st = avformat_new_stream(s, NULL);
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 5/9] nistspheredec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/nistspheredec.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c
index 782d1dfbfb..3386497682 100644
--- a/libavformat/nistspheredec.c
+++ b/libavformat/nistspheredec.c
@@ -21,6 +21,7 @@
 
 #include "libavutil/avstring.h"
 #include "libavutil/intreadwrite.h"
+#include "libavcodec/internal.h"
 #include "avformat.h"
 #include "internal.h"
 #include "pcm.h"
@@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s)
 return 0;
 } else if (!memcmp(buffer, "channel_count", 13)) {
 sscanf(buffer, "%*s %*s %"SCNd32, >codecpar->channels);
+if (st->codecpar->channels > FF_SANE_NB_CHANNELS) {
+av_log(s, AV_LOG_ERROR, "Too many channels %d > %d\n",
+   st->codecpar->channels, FF_SANE_NB_CHANNELS);
+return AVERROR(ENOSYS);
+}
 } else if (!memcmp(buffer, "sample_byte_format", 18)) {
 sscanf(buffer, "%*s %*s %31s", format);
 
@@ -109,6 +115,11 @@ static int nist_read_header(AVFormatContext *s)
 sscanf(buffer, "%*s %*s %"SCNd64, >duration);
 } else if (!memcmp(buffer, "sample_n_bytes", 14)) {
 sscanf(buffer, "%*s %*s %"SCNd32, );
+if (bps > (INT_MAX / FF_SANE_NB_CHANNELS) >> 3) {
+av_log(s, AV_LOG_ERROR, "Too many bytes per sample %d > %d\n",
+   bps, (INT_MAX / FF_SANE_NB_CHANNELS) >> 3);
+return AVERROR_INVALIDDATA;
+}
 } else if (!memcmp(buffer, "sample_rate", 11)) {
 sscanf(buffer, "%*s %*s %"SCNd32, >codecpar->sample_rate);
 } else if (!memcmp(buffer, "sample_sig_bits", 15)) {
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 4/9] ircamdec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/ircamdec.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/libavformat/ircamdec.c b/libavformat/ircamdec.c
index 59f3a49411..e3196db84a 100644
--- a/libavformat/ircamdec.c
+++ b/libavformat/ircamdec.c
@@ -20,6 +20,7 @@
  */
 
 #include "libavutil/intreadwrite.h"
+#include "libavcodec/internal.h"
 #include "avformat.h"
 #include "internal.h"
 #include "pcm.h"
@@ -87,6 +88,11 @@ static int ircam_read_header(AVFormatContext *s)
 
 st->codecpar->codec_type  = AVMEDIA_TYPE_AUDIO;
 st->codecpar->channels= channels;
+if (st->codecpar->channels > FF_SANE_NB_CHANNELS) {
+av_log(s, AV_LOG_ERROR, "Too many channels %d > %d\n",
+   st->codecpar->channels, FF_SANE_NB_CHANNELS);
+return AVERROR(ENOSYS);
+}
 st->codecpar->sample_rate = sample_rate;
 
 st->codecpar->codec_id = ff_codec_get_id(tags, tag);
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 3/9] genh: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/genh.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libavformat/genh.c b/libavformat/genh.c
index b683e026d1..dd4e76d3d9 100644
--- a/libavformat/genh.c
+++ b/libavformat/genh.c
@@ -20,6 +20,7 @@
  */
 
 #include "libavutil/intreadwrite.h"
+#include "libavcodec/internal.h"
 #include "avformat.h"
 #include "internal.h"
 
@@ -54,7 +55,7 @@ static int genh_read_header(AVFormatContext *s)
 
 st->codecpar->codec_type  = AVMEDIA_TYPE_AUDIO;
 st->codecpar->channels= avio_rl32(s->pb);
-if (st->codecpar->channels <= 0)
+if (st->codecpar->channels <= 0 || st->codecpar->channels > 
FF_SANE_NB_CHANNELS)
 return AVERROR_INVALIDDATA;
 if (st->codecpar->channels == 1)
 st->codecpar->channel_layout = AV_CH_LAYOUT_MONO;
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/9] electronicarts: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/electronicarts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/electronicarts.c b/libavformat/electronicarts.c
index 30eb723bd5..bfd3fed3a2 100644
--- a/libavformat/electronicarts.c
+++ b/libavformat/electronicarts.c
@@ -539,7 +539,7 @@ static int ea_read_header(AVFormatContext *s)
 ea->audio_codec = 0;
 return 1;
 }
-if (ea->bytes <= 0) {
+if (ea->bytes <= 0 || ea->bytes > 2) {
 av_log(s, AV_LOG_ERROR,
"Invalid number of bytes per sample: %d\n", ea->bytes);
 ea->audio_codec = AV_CODEC_ID_NONE;
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/9] 4xm: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/4xm.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavformat/4xm.c b/libavformat/4xm.c
index 2758b69d29..ead6d2b424 100644
--- a/libavformat/4xm.c
+++ b/libavformat/4xm.c
@@ -29,6 +29,7 @@
 
 #include "libavutil/intreadwrite.h"
 #include "libavutil/intfloat.h"
+#include "libavcodec/internal.h"
 #include "avformat.h"
 #include "internal.h"
 
@@ -153,8 +154,10 @@ static int parse_strk(AVFormatContext *s,
 fourxm->tracks[track].audio_pts   = 0;
 
 if (fourxm->tracks[track].channels<= 0 ||
+fourxm->tracks[track].channels > FF_SANE_NB_CHANNELS ||
 fourxm->tracks[track].sample_rate <= 0 ||
-fourxm->tracks[track].bits<= 0) {
+fourxm->tracks[track].bits<= 0 ||
+fourxm->tracks[track].bits > INT_MAX / FF_SANE_NB_CHANNELS) {
 av_log(s, AV_LOG_ERROR, "audio header invalid\n");
 return AVERROR_INVALIDDATA;
 }
-- 
2.11.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/9] electronicarts: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 07.01.2017 09:32, Paul B Mahol wrote:
> On 1/7/17, Michael Niedermayer <mich...@niedermayer.cc> wrote:
>> On Fri, Jan 06, 2017 at 08:47:39PM +0100, Andreas Cadhalpun wrote:
>>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>>> ---
>>>  libavformat/electronicarts.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/libavformat/electronicarts.c b/libavformat/electronicarts.c
>>> index 30eb723bd5..03422e5b2c 100644
>>> --- a/libavformat/electronicarts.c
>>> +++ b/libavformat/electronicarts.c
>>> @@ -556,6 +556,7 @@ static int ea_read_header(AVFormatContext *s)
>>>  st->codecpar->codec_tag = 0;   /* no tag */
>>>  st->codecpar->channels  = ea->num_channels;
>>>  st->codecpar->sample_rate   = ea->sample_rate;
>>> +FF_RETURN_ON_OVERFLOW(s, ea->bytes > INT_MAX / 8 / 2)
>>
>> I think we should ask for a sample when the number of bytes per
>> sample is larger than 2 or 4 or whatever max we think occurs.
> 
> No we should not as such samples are invalid.

The code seems to only know about 1 (8-bit) and 2 (16-bit), so
returning an error for larger values makes sense.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/9] 4xm: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 07.01.2017 02:31, Michael Niedermayer wrote:
> On Fri, Jan 06, 2017 at 09:27:29PM +0100, Andreas Cadhalpun wrote:
>>  4xm.c |1 +
>>  1 file changed, 1 insertion(+)
>> 4b27cb10f25865014fac1666956f7040d65113f9  
>> 0002-4xm-prevent-overflow-during-block-alignment-calculat.patch
>> From 861b62eec30feaa56b10eec7ba4029daf48a3c28 Mon Sep 17 00:00:00 2001
>> From: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> Date: Thu, 15 Dec 2016 02:14:31 +0100
>> Subject: [PATCH 2/9] 4xm: prevent overflow during block alignment calculation
>>
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavformat/4xm.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/libavformat/4xm.c b/libavformat/4xm.c
>> index 2758b69d29..58729fed0d 100644
>> --- a/libavformat/4xm.c
>> +++ b/libavformat/4xm.c
>> @@ -187,6 +187,7 @@ static int parse_strk(AVFormatContext *s,
>>  st->codecpar->bit_rate  = (int64_t)st->codecpar->channels *
>>st->codecpar->sample_rate *
>>
>> st->codecpar->bits_per_coded_sample;
>> +FF_RETURN_ON_OVERFLOW(s, st->codecpar->bits_per_coded_sample > INT_MAX 
>> / st->codecpar->channels)
>>  st->codecpar->block_align   = st->codecpar->channels *
>>
>> st->codecpar->bits_per_coded_sample;
> 
> i think we should check channels for > 8 or something and ask for a
> sample and check bits_per_coded_sample against what maximal sensible
> value of bits a sample and ask for a sample if above

Actually avcodec_open2 already errors out if channels is larger than
FF_SANE_NB_CHANNELS = 64. That check can already be done in the demuxer.
Then defining INT_MAX / 64 as maximal sensible value of bits_per_coded_sample
eliminates the need for FF_RETURN_ON_OVERFLOW checks.

I'll send an updated patch series.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] h264_ps: validate chroma sample location

[Andreas Cadhalpun]

On 07.01.2017 01:41, Michael Niedermayer wrote:
> On Fri, Jan 06, 2017 at 11:33:13PM +0100, Andreas Cadhalpun wrote:
>> On 06.01.2017 22:47, Kieran Kunhya wrote:
>>> On Fri, 6 Jan 2017 at 20:44 Andreas Cadhalpun <
>>> andreas.cadhal...@googlemail.com> wrote:
>>>
>>>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>>>> ---
>>>>  libavcodec/h264_ps.c | 4 
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
>>>> index 8218e3a010..089bfc650a 100644
>>>> --- a/libavcodec/h264_ps.c
>>>> +++ b/libavcodec/h264_ps.c
>>>> @@ -181,6 +181,10 @@ static inline int decode_vui_parameters(GetBitContext
>>>> *gb, AVCodecContext *avctx
>>>>  if (get_bits1(gb)) {
>>>>  /* chroma_sample_location_type_top_field */
>>>>  avctx->chroma_sample_location = get_ue_golomb(gb) + 1;
>>>> +if (!av_chroma_location_name(avctx->chroma_sample_location)) {
>>>> +av_log(avctx, AV_LOG_WARNING, "Invalid chroma sample location
>>>> %d, setting to unspecified\n", avctx->chroma_sample_location);
>>>> +avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED;
>>>> +}
>>>>
>>>>
>>> Is there a way to long only once, this seems like it could spam the user
>>> full of these warnings.
>>
>> One could add a field like shown_chroma_loc_warning to SPS, but I think
>> that would be a bit too much overhead for this.
>> Alternatively, one could drop the log message entirely. (Wrong color
>> primaries etc. aren't logged either...)
> 
> I think making it a "normally not displayed" log level would be a
> better choice than removing it entirely

OK, I reduced it to AV_LOG_VERBOSE.

Best regards,
Andreas
>From 5874739904fa8f13be03faee27e4bb2ac061258f Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
Date: Fri, 6 Jan 2017 21:36:39 +0100
Subject: [PATCH] h264_ps: validate chroma sample location

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavcodec/h264_ps.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
index 8218e3a010..474eaf0d2d 100644
--- a/libavcodec/h264_ps.c
+++ b/libavcodec/h264_ps.c
@@ -181,6 +181,10 @@ static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx
 if (get_bits1(gb)) {
 /* chroma_sample_location_type_top_field */
 avctx->chroma_sample_location = get_ue_golomb(gb) + 1;
+if (!av_chroma_location_name(avctx->chroma_sample_location)) {
+av_log(avctx, AV_LOG_VERBOSE, "Invalid chroma sample location %d, setting to unspecified\n", avctx->chroma_sample_location);
+avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED;
+}
 get_ue_golomb(gb);  /* chroma_sample_location_type_bottom_field */
 }
 
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] h264_ps: validate chroma sample location

[Andreas Cadhalpun]

On 06.01.2017 22:47, Kieran Kunhya wrote:
> On Fri, 6 Jan 2017 at 20:44 Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com> wrote:
> 
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavcodec/h264_ps.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
>> index 8218e3a010..089bfc650a 100644
>> --- a/libavcodec/h264_ps.c
>> +++ b/libavcodec/h264_ps.c
>> @@ -181,6 +181,10 @@ static inline int decode_vui_parameters(GetBitContext
>> *gb, AVCodecContext *avctx
>>  if (get_bits1(gb)) {
>>  /* chroma_sample_location_type_top_field */
>>  avctx->chroma_sample_location = get_ue_golomb(gb) + 1;
>> +if (!av_chroma_location_name(avctx->chroma_sample_location)) {
>> +av_log(avctx, AV_LOG_WARNING, "Invalid chroma sample location
>> %d, setting to unspecified\n", avctx->chroma_sample_location);
>> +avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED;
>> +}
>>
>>
> Is there a way to long only once, this seems like it could spam the user
> full of these warnings.

One could add a field like shown_chroma_loc_warning to SPS, but I think
that would be a bit too much overhead for this.
Alternatively, one could drop the log message entirely. (Wrong color
primaries etc. aren't logged either...)

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 6/9] nistspheredec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 06.01.2017 22:30, Ronald S. Bultje wrote:
> On Fri, Jan 6, 2017 at 2:48 PM, Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com> wrote:
> 
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavformat/nistspheredec.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c
>> index 782d1dfbfb..9023ed7fc7 100644
>> --- a/libavformat/nistspheredec.c
>> +++ b/libavformat/nistspheredec.c
>> @@ -80,6 +80,7 @@ static int nist_read_header(AVFormatContext *s)
>>
>>  avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
>>
>> +FF_RETURN_ON_OVERFLOW(s, st->codecpar->channels &&
>> st->codecpar->bits_per_coded_sample > INT_MAX / st->codecpar->channels)
> 
> 
> Same comment as the other one, the channels == 0 isn't a valid case that we
> want to special case, probably check that it's not zero separately.

Here I disagree: This is only the demuxer, that doesn't need to know
the number of channels, which can be set later by the decoder.
(For example the shorten decoder does this.)

Thus I think this check here is really needed.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 5/9] ircamdec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 06.01.2017 22:31, Ronald S. Bultje wrote:
> On Fri, Jan 6, 2017 at 2:48 PM, Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com> wrote:
> 
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavformat/ircamdec.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/libavformat/ircamdec.c b/libavformat/ircamdec.c
>> index 59f3a49411..f3cf4d0dc9 100644
>> --- a/libavformat/ircamdec.c
>> +++ b/libavformat/ircamdec.c
>> @@ -96,6 +96,7 @@ static int ircam_read_header(AVFormatContext *s)
>>  }
>>
>>  st->codecpar->bits_per_coded_sample = av_get_bits_per_sample(st->
>> codecpar->codec_id);
>> +FF_RETURN_ON_OVERFLOW(s, st->codecpar->channels &&
>> st->codecpar->bits_per_coded_sample > INT_MAX / st->codecpar->channels)
>>  st->codecpar->block_align = st->codecpar->bits_per_coded_sample *
>> st->codecpar->channels / 8;
>>  avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
>>  avio_skip(s->pb, 1008);
> 
> 
> I see this code a few lines up:
> 
> if (!channels || !sample_rate)
>     return AVERROR_INVALIDDATA;
> 
> So channels == 0 seems impossible to me.

Right, I dropped the check for that.

Best regards,
Andreas

>From b91a25e4b8a79d8d39a9c0593d0715190474a4ec Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
Date: Thu, 15 Dec 2016 02:14:45 +0100
Subject: [PATCH 5/9] ircamdec: prevent overflow during block alignment
 calculation

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/ircamdec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/ircamdec.c b/libavformat/ircamdec.c
index 59f3a49411..5d2d0ab9b9 100644
--- a/libavformat/ircamdec.c
+++ b/libavformat/ircamdec.c
@@ -96,6 +96,7 @@ static int ircam_read_header(AVFormatContext *s)
 }
 
 st->codecpar->bits_per_coded_sample = av_get_bits_per_sample(st->codecpar->codec_id);
+FF_RETURN_ON_OVERFLOW(s, st->codecpar->bits_per_coded_sample > INT_MAX / st->codecpar->channels)
 st->codecpar->block_align = st->codecpar->bits_per_coded_sample * st->codecpar->channels / 8;
 avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
 avio_skip(s->pb, 1008);
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] h264_ps: validate chroma sample location

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavcodec/h264_ps.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
index 8218e3a010..089bfc650a 100644
--- a/libavcodec/h264_ps.c
+++ b/libavcodec/h264_ps.c
@@ -181,6 +181,10 @@ static inline int decode_vui_parameters(GetBitContext *gb, 
AVCodecContext *avctx
 if (get_bits1(gb)) {
 /* chroma_sample_location_type_top_field */
 avctx->chroma_sample_location = get_ue_golomb(gb) + 1;
+if (!av_chroma_location_name(avctx->chroma_sample_location)) {
+av_log(avctx, AV_LOG_WARNING, "Invalid chroma sample location %d, 
setting to unspecified\n", avctx->chroma_sample_location);
+avctx->chroma_sample_location = AVCHROMA_LOC_UNSPECIFIED;
+}
 get_ue_golomb(gb);  /* chroma_sample_location_type_bottom_field */
 }
 
-- 
2.11.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] mpeg12dec: validate color space

[Andreas Cadhalpun]

On 23.12.2016 00:57, Andreas Cadhalpun wrote:
> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
> ---
>  libavcodec/mpeg12dec.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> index 63979079c8..d3dc67ad6a 100644
> --- a/libavcodec/mpeg12dec.c
> +++ b/libavcodec/mpeg12dec.c
> @@ -1470,6 +1470,10 @@ static void 
> mpeg_decode_sequence_display_extension(Mpeg1Context *s1)
>  s->avctx->color_primaries = get_bits(>gb, 8);
>  s->avctx->color_trc   = get_bits(>gb, 8);
>  s->avctx->colorspace  = get_bits(>gb, 8);
> +if (!av_color_space_name(s->avctx->colorspace)) {
> +av_log(s->avctx, AV_LOG_WARNING, "Invalid color space %d, 
> setting to unspecified\n", s->avctx->colorspace);
> +s->avctx->colorspace = AVCOL_SPC_UNSPECIFIED;
> +}
>  }
>  w = get_bits(>gb, 14);
>  skip_bits(>gb, 1); // marker
> 

Ping for the series.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/9] 4xm: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 06.01.2017 20:58, Ronald S. Bultje wrote:
> Hi,
> 
> On Fri, Jan 6, 2017 at 2:47 PM, Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com> wrote:
> 
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavformat/4xm.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/libavformat/4xm.c b/libavformat/4xm.c
>> index 2758b69d29..45949c4e97 100644
>> --- a/libavformat/4xm.c
>> +++ b/libavformat/4xm.c
>> @@ -187,6 +187,7 @@ static int parse_strk(AVFormatContext *s,
>>  st->codecpar->bit_rate  = (int64_t)st->codecpar->channels
>> *
>>st->codecpar->sample_rate *
>>st->codecpar->bits_per_coded_
>> sample;
>> +FF_RETURN_ON_OVERFLOW(s, st->codecpar->channels &&
>> st->codecpar->bits_per_coded_sample > INT_MAX / st->codecpar->channels)
>>  st->codecpar->block_align   = st->codecpar->channels *
>>st->codecpar->bits_per_coded_
>> sample;
>>
>> --
>> 2.11.0
> 
> 
> To an innocent reader (who doesn't know/care about SIGFPE), this might look
> like channels = 0 is an actual valid decoder condition that is explicitly
> handled here.

Actually this function errors out earlier if channels is zero, so I've removed
this pointless additional check. Updated patch is attached.

Best regards,
Andreas


>From 861b62eec30feaa56b10eec7ba4029daf48a3c28 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
Date: Thu, 15 Dec 2016 02:14:31 +0100
Subject: [PATCH 2/9] 4xm: prevent overflow during block alignment calculation

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/4xm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/4xm.c b/libavformat/4xm.c
index 2758b69d29..58729fed0d 100644
--- a/libavformat/4xm.c
+++ b/libavformat/4xm.c
@@ -187,6 +187,7 @@ static int parse_strk(AVFormatContext *s,
 st->codecpar->bit_rate  = (int64_t)st->codecpar->channels *
   st->codecpar->sample_rate *
   st->codecpar->bits_per_coded_sample;
+FF_RETURN_ON_OVERFLOW(s, st->codecpar->bits_per_coded_sample > INT_MAX / st->codecpar->channels)
 st->codecpar->block_align   = st->codecpar->channels *
   st->codecpar->bits_per_coded_sample;
 
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/9] 4xm: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/4xm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/4xm.c b/libavformat/4xm.c
index 2758b69d29..45949c4e97 100644
--- a/libavformat/4xm.c
+++ b/libavformat/4xm.c
@@ -187,6 +187,7 @@ static int parse_strk(AVFormatContext *s,
 st->codecpar->bit_rate  = (int64_t)st->codecpar->channels *
   st->codecpar->sample_rate *
   st->codecpar->bits_per_coded_sample;
+FF_RETURN_ON_OVERFLOW(s, st->codecpar->channels && 
st->codecpar->bits_per_coded_sample > INT_MAX / st->codecpar->channels)
 st->codecpar->block_align   = st->codecpar->channels *
   st->codecpar->bits_per_coded_sample;
 
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 8/9] epafdec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

---
 libavformat/epafdec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/epafdec.c b/libavformat/epafdec.c
index 29190fff72..b28f035cdf 100644
--- a/libavformat/epafdec.c
+++ b/libavformat/epafdec.c
@@ -83,6 +83,7 @@ static int epaf_read_header(AVFormatContext *s)
 }
 
 st->codecpar->bits_per_coded_sample = 
av_get_bits_per_sample(st->codecpar->codec_id);
+FF_RETURN_ON_OVERFLOW(s, st->codecpar->bits_per_coded_sample > INT_MAX / 
st->codecpar->channels)
 st->codecpar->block_align = st->codecpar->bits_per_coded_sample * 
st->codecpar->channels / 8;
 
 avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 9/9] xvag: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/xvag.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/xvag.c b/libavformat/xvag.c
index 5ef4fb0900..1f28df7b89 100644
--- a/libavformat/xvag.c
+++ b/libavformat/xvag.c
@@ -74,6 +74,7 @@ static int xvag_read_header(AVFormatContext *s)
 switch (codec) {
 case 0x1c:
 st->codecpar->codec_id= AV_CODEC_ID_ADPCM_PSX;
+FF_RETURN_ON_OVERFLOW(s, st->codecpar->channels > INT_MAX / 16)
 st->codecpar->block_align = 16 * st->codecpar->channels;
 break;
 default:
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 6/9] nistspheredec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/nistspheredec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c
index 782d1dfbfb..9023ed7fc7 100644
--- a/libavformat/nistspheredec.c
+++ b/libavformat/nistspheredec.c
@@ -80,6 +80,7 @@ static int nist_read_header(AVFormatContext *s)
 
 avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
 
+FF_RETURN_ON_OVERFLOW(s, st->codecpar->channels && 
st->codecpar->bits_per_coded_sample > INT_MAX / st->codecpar->channels)
 st->codecpar->block_align = st->codecpar->bits_per_coded_sample * 
st->codecpar->channels / 8;
 
 if (avio_tell(s->pb) > header_size)
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 7/9] pvfdec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/pvfdec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/pvfdec.c b/libavformat/pvfdec.c
index b9f6d4f2c2..94ef20c249 100644
--- a/libavformat/pvfdec.c
+++ b/libavformat/pvfdec.c
@@ -56,6 +56,7 @@ static int pvf_read_header(AVFormatContext *s)
 st->codecpar->sample_rate = sample_rate;
 st->codecpar->codec_id= ff_get_pcm_codec_id(bps, 0, 1, 0x);
 st->codecpar->bits_per_coded_sample = bps;
+FF_RETURN_ON_OVERFLOW(s, bps > INT_MAX / st->codecpar->channels)
 st->codecpar->block_align = bps * st->codecpar->channels / 8;
 
 avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 5/9] ircamdec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/ircamdec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/ircamdec.c b/libavformat/ircamdec.c
index 59f3a49411..f3cf4d0dc9 100644
--- a/libavformat/ircamdec.c
+++ b/libavformat/ircamdec.c
@@ -96,6 +96,7 @@ static int ircam_read_header(AVFormatContext *s)
 }
 
 st->codecpar->bits_per_coded_sample = 
av_get_bits_per_sample(st->codecpar->codec_id);
+FF_RETURN_ON_OVERFLOW(s, st->codecpar->channels && 
st->codecpar->bits_per_coded_sample > INT_MAX / st->codecpar->channels)
 st->codecpar->block_align = st->codecpar->bits_per_coded_sample * 
st->codecpar->channels / 8;
 avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
 avio_skip(s->pb, 1008);
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 4/9] genh: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/genh.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/genh.c b/libavformat/genh.c
index b683e026d1..6ce2588ed3 100644
--- a/libavformat/genh.c
+++ b/libavformat/genh.c
@@ -74,6 +74,7 @@ static int genh_read_header(AVFormatContext *s)
 case  0: st->codecpar->codec_id = AV_CODEC_ID_ADPCM_PSX;break;
 case  1:
 case 11: st->codecpar->bits_per_coded_sample = 4;
+ FF_RETURN_ON_OVERFLOW(s, st->codecpar->channels > INT_MAX / 36)
  st->codecpar->block_align = 36 * st->codecpar->channels;
  st->codecpar->codec_id = AV_CODEC_ID_ADPCM_IMA_WAV;break;
 case  2: st->codecpar->codec_id = AV_CODEC_ID_ADPCM_DTK;break;
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 3/9] electronicarts: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/electronicarts.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/electronicarts.c b/libavformat/electronicarts.c
index 30eb723bd5..03422e5b2c 100644
--- a/libavformat/electronicarts.c
+++ b/libavformat/electronicarts.c
@@ -556,6 +556,7 @@ static int ea_read_header(AVFormatContext *s)
 st->codecpar->codec_tag = 0;   /* no tag */
 st->codecpar->channels  = ea->num_channels;
 st->codecpar->sample_rate   = ea->sample_rate;
+FF_RETURN_ON_OVERFLOW(s, ea->bytes > INT_MAX / 8 / 2)
 st->codecpar->bits_per_coded_sample = ea->bytes * 8;
 st->codecpar->bit_rate  = (int64_t)st->codecpar->channels *
   st->codecpar->sample_rate *
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/9] avutil: add FF_RETURN_ON_OVERFLOW

[Andreas Cadhalpun]

Suggested-by: Rodger Combs <rodger.co...@gmail.com>
Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---

Changed the name as suggested by wm4 and the return value as suggested
by Muhammad Faiz.
There are also two new overflow checks at the end of the series.

---
 libavutil/common.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavutil/common.h b/libavutil/common.h
index 8142b31fdb..6d795a353a 100644
--- a/libavutil/common.h
+++ b/libavutil/common.h
@@ -99,6 +99,8 @@
 #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)
 #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
 
+#define FF_RETURN_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR, 
"Overflow check failed: " #x"\n"); return AVERROR(ERANGE);}
+
 /* misc math functions */
 
 #ifdef HAVE_AV_CONFIG_H
-- 
2.11.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] opt: check image size when setting it

[Andreas Cadhalpun]

On 11.12.2016 21:45, Andreas Cadhalpun wrote:
> On 11.12.2016 21:03, Hendrik Leppkes wrote:
>> On Sun, Dec 11, 2016 at 7:48 PM, Andreas Cadhalpun
>> <andreas.cadhal...@googlemail.com> wrote:
>>> On 11.12.2016 10:04, Hendrik Leppkes wrote:
>>>> I still see the problem that this option code does not know which
>>>> pix_fmt the numbers relate to and as such would keep the old limit in
>>>> place despite there being no technical reasons for it.
>>>
>>> And I still think that av_image_check_size should be changed to
>>> accept the largest value valid for any pixel format (once every place
>>> needing stricter limits is switched to the pixel format sensitive
>>> check).
>>> Do you disagree with this or what is your point?
>>
>> I could probably live with a simple w*h overflow check (more or less),
>> which av_image_check_size then probably would end up being if I
>> understand you right?
> 
> I don't think so. For example, av_image_check_size2 accepts resolutions
> like 10x8 for AV_PIX_FMT_MONOWHITE and thus av_image_check_size
> should also accept this, even though the number of pixels is larger
> than INT_MAX. However, that's not the current state of affairs, so
> until the work is done to actually use the pixel format specific limits,
> the option code should check for the old limit.
> 
>> Thats much higher then the current limit in most cases and while we
>> should try to move this to size_t/ptrdiff_t eventually to lift the
>> limit even higher on 64-bit platforms, its OK for now.
> 
> Note that av_image_check_size is documented to check that
> "all bytes of the image can be addressed with a signed int",
> so increasing the limit higher requires using a different function.

I assume I've convinced you, so I'll apply this patch soon, unless
I hear back from you.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] omadec: fix overflows during bit rate calculation

[Andreas Cadhalpun]

On 15.12.2016 01:53, Andreas Cadhalpun wrote:
> On 14.12.2016 11:16, Moritz Barsnick wrote:
>> On Wed, Dec 14, 2016 at 01:02:41 +0100, Andreas Cadhalpun wrote:
>>> On 13.12.2016 08:11, Paul B Mahol wrote:
>>>>> -st->codecpar->bit_rate= samplerate * framesize * 8 / 2048;
>>>>> +st->codecpar->bit_rate= samplerate * framesize / 256;
>>>
>>> Why multiply with 8 when dividing by a multiple of 8 directly afterwards?
>>> That's just a waste of computational resources.
>>
>> I can only explain the term with "readability" (e.g. "number of bytes
>> times 8 is number of bits, divided by 2048 is the rate"). If you
>> bracket the (8 / 2048), it would avoid the overflow, and the compiler
>> should evaluate the term to that constant 256 anyway, right? (Just if
>> anyone cares about the presumed readability.)
> 
> Well, (8 / 2048) = 0, but one can do "/ (2048 / 8)".
> Attached is a version doing it that way. Do you think that's better?

I've pushed this variant now.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] wmavoice: truncate spillover_nbits if too large

[Andreas Cadhalpun]

On 02.01.2017 04:14, Ronald S. Bultje wrote:
> On Sun, Jan 1, 2017 at 5:55 PM, Andreas Cadhalpun 
> <andreas.cadhal...@googlemail.com> wrote:
> So what would you do instead?
> 
> I'd just remove the message, but otherwise what you're doing (truncate 
> spillover_nbits) seems fine.

OK. While I think that the message could have been useful, the important thing 
is that the code
doesn't crash. So I've just pushed the patch without the log message.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] wmavoice: prevent division by zero crash

[Andreas Cadhalpun]

On 02.01.2017 04:09, Ronald S. Bultje wrote:
> On Sun, Jan 1, 2017 at 5:51 PM, Andreas Cadhalpun 
> <andreas.cadhal...@googlemail.com> wrote:
> Fine for me. Patch doing that is attached.
> 
> 
> LGTM.

Pushed.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] wmavoice: truncate spillover_nbits if too large

[Andreas Cadhalpun]

On 01.01.2017 23:27, Ronald S. Bultje wrote:
> On Sun, Jan 1, 2017 at 5:18 PM, Andreas Cadhalpun 
> <andreas.cadhal...@googlemail.com <mailto:andreas.cadhal...@googlemail.com>> 
> wrote:
> 
> This fixes triggering the av_assert0(ret <= tmp.size).
> 
> The problem was reintroduced by commit
> 7b27dd5c16de785297ce4de4b88afa0b6685f61d and originally fixed by
> 2a4700a4f03280fa8ba4fc0f8a9987bb550f0d1e.
> 
> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com 
> <mailto:andreas.cadhal...@googlemail.com>>
> ---
>  libavcodec/wmavoice.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
> index cd5958c7bc..1bfad46b2e 100644
> --- a/libavcodec/wmavoice.c
> +++ b/libavcodec/wmavoice.c
> @@ -1923,6 +1923,11 @@ static int wmavoice_decode_packet(AVCodecContext 
> *ctx, void *data,
>   * continuing to parse new superframes in the current packet. */
>  if (s->sframe_cache_size > 0) {
>  int cnt = get_bits_count(gb);
> +if (cnt + s->spillover_nbits > avpkt->size * 8) {
> +av_log(ctx, AV_LOG_WARNING, "Number of spillover bits %d 
> larger than remaining packet size %d, truncating.\n",
> +   s->spillover_nbits, avpkt->size * 8 - cnt);
> +s->spillover_nbits = avpkt->size * 8 - cnt;
> +}
> 
> 
> This litters stderr uselessly. It doesn't help a user or developer accomplish
> anything. Let me explain, because this is probably confusing.
> 
> If a library integrator (e.g., the developer of gst-ffmpeg) is playing with
> wmavoice and forgets to set block_align, the codec should error out. Ideally,
> it does this saying block_align is zero, so the developer knows what to do.
> But this is not strictly required and we typically don't do this.
> 
> However, the above can only happen during corrupt streams (or fuzzing).
> The user cannot fix this. So any detailed error is useless.

I don't think it's useless, as it tells the user that there is a problem with
the file. He might just test if ffmpeg can decode it to see if it is corrupt.

> Therefore, we should not display any detailed error (or warning) message.

So what would you do instead?

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] wmavoice: prevent division by zero crash

[Andreas Cadhalpun]

On 01.01.2017 23:23, Ronald S. Bultje wrote:
> On Sun, Jan 1, 2017 at 5:19 PM, Andreas Cadhalpun 
> <andreas.cadhal...@googlemail.com <mailto:andreas.cadhal...@googlemail.com>> 
> wrote:
> 
> The problem was introduced by commit
> 3deb4b54a24f8cddce463d9f5751b01efeb976af.
> 
> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com 
> <mailto:andreas.cadhal...@googlemail.com>>
> ---
>  libavcodec/wmavoice.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
> index 1bfad46b2e..279b44dc12 100644
> --- a/libavcodec/wmavoice.c
> +++ b/libavcodec/wmavoice.c
> @@ -1908,7 +1908,7 @@ static int wmavoice_decode_packet(AVCodecContext 
> *ctx, void *data,
>  /* size == ctx->block_align is used to indicate whether we are 
> dealing with
>   * a new packet or a packet of which we already read the packet 
> header
>   * previously. */
> -if (!(size % ctx->block_align)) { // new packet header
> +if (ctx->block_align && !(size % ctx->block_align)) { // new packet 
> header
>  if (!size) {
>  s->spillover_nbits = 0;
>  s->nb_superframes = 0;
> --
> 2.11.0
> 
> 
> nak.
> 
> The init routine should error out if block_align is zero.
> The codec can not operate without block_align set.

Fine for me. Patch doing that is attached.

Best regards,
Andreas

>From caec0e9f57ddc2373d3e2cb56ed1e6c3ce0df166 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
Date: Sun, 1 Jan 2017 22:48:38 +0100
Subject: [PATCH] wmavoice: validate block alignment

This prevents a division by zero crash in wmavoice_decode_packet.

The problem was introduced by commit
3deb4b54a24f8cddce463d9f5751b01efeb976af.

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavcodec/wmavoice.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index 1bfad46b2e..080ec86b53 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -388,6 +388,11 @@ static av_cold int wmavoice_decode_init(AVCodecContext *ctx)
ctx->extradata_size);
 return AVERROR_INVALIDDATA;
 }
+if (ctx->block_align <= 0) {
+av_log(ctx, AV_LOG_ERROR, "Invalid block alignment %d.\n", ctx->block_align);
+return AVERROR_INVALIDDATA;
+}
+
 flags= AV_RL32(ctx->extradata + 18);
 s->spillover_bitsize = 3 + av_ceil_log2(ctx->block_align);
 s->do_apf=flags & 0x1;
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/2] wmavoice: truncate spillover_nbits if too large

[Andreas Cadhalpun]

This fixes triggering the av_assert0(ret <= tmp.size).

The problem was reintroduced by commit
7b27dd5c16de785297ce4de4b88afa0b6685f61d and originally fixed by
2a4700a4f03280fa8ba4fc0f8a9987bb550f0d1e.

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavcodec/wmavoice.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index cd5958c7bc..1bfad46b2e 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -1923,6 +1923,11 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, 
void *data,
  * continuing to parse new superframes in the current packet. */
 if (s->sframe_cache_size > 0) {
 int cnt = get_bits_count(gb);
+if (cnt + s->spillover_nbits > avpkt->size * 8) {
+av_log(ctx, AV_LOG_WARNING, "Number of spillover bits %d 
larger than remaining packet size %d, truncating.\n",
+   s->spillover_nbits, avpkt->size * 8 - cnt);
+s->spillover_nbits = avpkt->size * 8 - cnt;
+}
 copy_bits(>pb, avpkt->data, size, gb, s->spillover_nbits);
 flush_put_bits(>pb);
 s->sframe_cache_size += s->spillover_nbits;
-- 
2.11.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/2] wmavoice: prevent division by zero crash

[Andreas Cadhalpun]

The problem was introduced by commit
3deb4b54a24f8cddce463d9f5751b01efeb976af.

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavcodec/wmavoice.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index 1bfad46b2e..279b44dc12 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -1908,7 +1908,7 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, 
void *data,
 /* size == ctx->block_align is used to indicate whether we are dealing with
  * a new packet or a packet of which we already read the packet header
  * previously. */
-if (!(size % ctx->block_align)) { // new packet header
+if (ctx->block_align && !(size % ctx->block_align)) { // new packet header
 if (!size) {
 s->spillover_nbits = 0;
 s->nb_superframes = 0;
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] libopenmpt: add missing avio_read return value check

[Andreas Cadhalpun]

This fixes heap-buffer-overflows in libopenmpt caused by interpreting
the negative size value as unsigned size_t.

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/libopenmpt.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/libavformat/libopenmpt.c b/libavformat/libopenmpt.c
index e7091ef9fc..35fd28f5f4 100644
--- a/libavformat/libopenmpt.c
+++ b/libavformat/libopenmpt.c
@@ -82,6 +82,11 @@ static int read_header_openmpt(AVFormatContext *s)
 if (!buf)
 return AVERROR(ENOMEM);
 size = avio_read(s->pb, buf, size);
+if (size < 0) {
+av_log(s, AV_LOG_ERROR, "Reading input buffer failed.\n");
+av_freep();
+return size;
+}
 
 openmpt->module = openmpt_module_create_from_memory(buf, size, 
openmpt_logfunc, s, NULL);
 av_freep();
-- 
2.11.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [FFmpeg-cvslog] avcodec: add Apple Pixlet decoder

[Andreas Cadhalpun]

On 23.12.2016 09:17, Paul B Mahol wrote:
> On 12/23/16, Andreas Cadhalpun <andreas.cadhal...@googlemail.com> wrote:
>> On 22.12.2016 22:52, Paul B Mahol wrote:
>>> +xflag = flag + cnt1;
>>> +yflag = xflag;
>>> +
>>> +if (flag + cnt1 == 0) {
>>> +value = 0;
>>> +} else {
>>> +xflag &= 1u;
>>> +tmp = c * ((yflag + 1) >> 1) + (c >> 1);
>>
>> This can overflow.
> 
> And?

That is undefined behavior. It looks like at least a cast to int64_t is missing.

>>> +value = xflag + (tmp ^ -xflag);
>>> +}
>>> +
>>> +i++;
>>> +dst[j++] = value;
>>> +if (j == width) {
>>> +j = 0;
>>> +dst += stride;
>>> +}
>>> +state += d * yflag - (d * state >> 8);
>>
>> This can overflow, too.
> 
> And?

Same as above.

>> [...]
>>> +static void postprocess_chroma(AVFrame *frame, int w, int h, int depth)
>>> +{
>>> +uint16_t *dstu = (uint16_t *)frame->data[1];
>>> +uint16_t *dstv = (uint16_t *)frame->data[2];
>>> +int16_t *srcu  = (int16_t *)frame->data[1];
>>> +int16_t *srcv  = (int16_t *)frame->data[2];
>>> +ptrdiff_t strideu = frame->linesize[1] / 2;
>>> +ptrdiff_t stridev = frame->linesize[2] / 2;
>>> +const int add = 1 << (depth - 1);
>>> +const int shift = 16 - depth;
>>> +int i, j;
>>> +
>>> +for (j = 0; j < h; j++) {
>>> +for (i = 0; i < w; i++) {
>>> +dstu[i] = (add + srcu[i]) << shift;
>>> +dstv[i] = (add + srcv[i]) << shift;
>>
>> These result in undefined shifts, since the value to be shifted can be
>> negative.
> 
> OK.

Making add unsigned would fix that.

>>> +}
>>> +dstu += strideu;
>>> +dstv += stridev;
>>> +srcu += strideu;
>>> +srcv += stridev;
>>> +}
>>> +}
>>> +
>>> +static int decode_plane(AVCodecContext *avctx, int plane, AVPacket
>>> *avpkt, AVFrame *frame)
>>> +{
>>> +PixletContext *ctx = avctx->priv_data;
>>> +ptrdiff_t stride = frame->linesize[plane] / 2;
>>> +unsigned shift = plane > 0;
>>> +int16_t *dst;
>>> +int i, ret;
>>> +
>>> +for (i = ctx->levels - 1; i >= 0; i--) {
>>> +ctx->scaling[plane][H][i] = 100. /
>>> sign_extend(bytestream2_get_be32(>gb), 32);
>>> +ctx->scaling[plane][V][i] = 100. /
>>> sign_extend(bytestream2_get_be32(>gb), 32);
>>
>> Here the denominators can be zero.
> 
> And?

That is undefined behavior.

>>> +*got_frame = 1;
>>> +
>>> +return pktsize;
>>
>> Since this is a video decoder, this should always return the full
>> avpkt->size.
> 
> Wrong. It returns what it consumes.

Video decoders are expected to consume the full packet and the new decoding API 
does that.
So not returning the full packet size results in a behavior difference between 
the new
and the old API, which is bad. Why would you want that?

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [FFmpeg-cvslog] avcodec: add Apple Pixlet decoder

[Andreas Cadhalpun]

On 22.12.2016 22:52, Paul B Mahol wrote:
> ffmpeg | branch: master | Paul B Mahol  | Fri Dec  2 
> 20:30:50 2016 +0100| [73651090ca1183f37753ee30a7e206ca4fb9f4f0] | committer: 
> Paul B Mahol
> 
> avcodec: add Apple Pixlet decoder
> 
> Signed-off-by: Paul B Mahol 
> 
>> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=73651090ca1183f37753ee30a7e206ca4fb9f4f0
> ---
> 
>  Changelog   |   1 +
>  doc/general.texi|   1 +
>  libavcodec/Makefile |   1 +
>  libavcodec/allcodecs.c  |   1 +
>  libavcodec/avcodec.h|   1 +
>  libavcodec/codec_desc.c |   7 +
>  libavcodec/pixlet.c | 672 
> 
>  libavcodec/version.h|   2 +-
>  libavformat/isom.c  |   2 +
>  9 files changed, 687 insertions(+), 1 deletion(-)
[...]
> +static int read_high_coeffs(AVCodecContext *avctx, uint8_t *src, int16_t 
> *dst, int size,
> +int c, int a, int d,
> +int width, ptrdiff_t stride)
> +{
> +PixletContext *ctx = avctx->priv_data;
> +GetBitContext *b = >gbit;
> +unsigned cnt1, shbits, rlen, nbits, length, i = 0, j = 0, k;
> +int ret, escape, pfx, value, yflag, xflag, flag = 0;
> +int64_t state = 3, tmp;
> +
> +if ((ret = init_get_bits8(b, src, bytestream2_get_bytes_left(>gb))) 
> < 0)
> +  return ret;
> +
> +if ((a >= 0) + (a ^ (a >> 31)) - (a >> 31) != 1) {
> +nbits = 33 - ff_clz((a >= 0) + (a ^ (a >> 31)) - (a >> 31) - 1);
> +if (nbits > 16)
> +return AVERROR_INVALIDDATA;
> +} else {
> +nbits = 1;
> +}
> +
> +length = 25 - nbits;
> +
> +while (i < size) {
> +if (state >> 8 != -3) {
> +value = ff_clz((state >> 8) + 3) ^ 0x1F;
> +} else {
> +value = -1;
> +}
> +
> +cnt1 = get_unary(b, 0, length);
> +
> +if (cnt1 >= length) {
> +cnt1 = get_bits(b, nbits);
> +} else {
> +pfx = 14 + uint64_t)(value - 14)) >> 32) & (value - 14));
> +cnt1 *= (1 << pfx) - 1;
> +shbits = show_bits(b, pfx);
> +if (shbits <= 1) {
> +skip_bits(b, pfx - 1);
> +} else {
> +skip_bits(b, pfx);
> +cnt1 += shbits - 1;
> +}
> +}
> +
> +xflag = flag + cnt1;
> +yflag = xflag;
> +
> +if (flag + cnt1 == 0) {
> +value = 0;
> +} else {
> +xflag &= 1u;
> +tmp = c * ((yflag + 1) >> 1) + (c >> 1);

This can overflow.

> +value = xflag + (tmp ^ -xflag);
> +}
> +
> +i++;
> +dst[j++] = value;
> +if (j == width) {
> +j = 0;
> +dst += stride;
> +}
> +state += d * yflag - (d * state >> 8);

This can overflow, too.

> +
> +flag = 0;
> +
> +if (state * 4 > 0xFF || i >= size)
> +continue;
> +
> +pfx = ((state + 8) >> 5) + (state ? ff_clz(state): 32) - 24;
> +escape = av_mod_uintp2(16383, pfx);

Here pfx can be negative, but av_mod_uintp2 takes an unsigned argument, so it's
interpreted as very large exponent, causing undefined shifts.

> +cnt1 = get_unary(b, 0, 8);
> +if (cnt1 < 8) {
> +value = show_bits(b, pfx);

And a negative pfx triggers an assertion in show_bits.

[...]
> +static void postprocess_chroma(AVFrame *frame, int w, int h, int depth)
> +{
> +uint16_t *dstu = (uint16_t *)frame->data[1];
> +uint16_t *dstv = (uint16_t *)frame->data[2];
> +int16_t *srcu  = (int16_t *)frame->data[1];
> +int16_t *srcv  = (int16_t *)frame->data[2];
> +ptrdiff_t strideu = frame->linesize[1] / 2;
> +ptrdiff_t stridev = frame->linesize[2] / 2;
> +const int add = 1 << (depth - 1);
> +const int shift = 16 - depth;
> +int i, j;
> +
> +for (j = 0; j < h; j++) {
> +for (i = 0; i < w; i++) {
> +dstu[i] = (add + srcu[i]) << shift;
> +dstv[i] = (add + srcv[i]) << shift;

These result in undefined shifts, since the value to be shifted can be negative.

> +}
> +dstu += strideu;
> +dstv += stridev;
> +srcu += strideu;
> +srcv += stridev;
> +}
> +}
> +
> +static int decode_plane(AVCodecContext *avctx, int plane, AVPacket *avpkt, 
> AVFrame *frame)
> +{
> +PixletContext *ctx = avctx->priv_data;
> +ptrdiff_t stride = frame->linesize[plane] / 2;
> +unsigned shift = plane > 0;
> +int16_t *dst;
> +int i, ret;
> +
> +for (i = ctx->levels - 1; i >= 0; i--) {
> +ctx->scaling[plane][H][i] = 100. / 
> sign_extend(bytestream2_get_be32(>gb), 32);
> +ctx->scaling[plane][V][i] = 100. / 
> sign_extend(bytestream2_get_be32(>gb), 32);

Here the denominators can be zero.

> +static int pixlet_decode_frame(AVCodecContext *avctx, void *data,
> +   int 

[FFmpeg-devel] [PATCH 2/3] mpeg12dec: validate color primaries

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavcodec/mpeg12dec.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index d3dc67ad6a..89aecd4de4 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -1468,6 +1468,10 @@ static void 
mpeg_decode_sequence_display_extension(Mpeg1Context *s1)
 color_description = get_bits1(>gb);
 if (color_description) {
 s->avctx->color_primaries = get_bits(>gb, 8);
+if (!av_color_primaries_name(s->avctx->color_primaries)) {
+av_log(s->avctx, AV_LOG_WARNING, "Invalid color primaries %d, 
setting to unspecified\n", s->avctx->color_primaries);
+s->avctx->color_primaries = AVCOL_PRI_UNSPECIFIED;
+}
 s->avctx->color_trc   = get_bits(>gb, 8);
 s->avctx->colorspace  = get_bits(>gb, 8);
 if (!av_color_space_name(s->avctx->colorspace)) {
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 3/3] mpeg12dec: validate color transfer characteristics

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavcodec/mpeg12dec.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 89aecd4de4..964f498f3b 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -1473,6 +1473,10 @@ static void 
mpeg_decode_sequence_display_extension(Mpeg1Context *s1)
 s->avctx->color_primaries = AVCOL_PRI_UNSPECIFIED;
 }
 s->avctx->color_trc   = get_bits(>gb, 8);
+if (!av_color_transfer_name(s->avctx->color_trc)) {
+av_log(s->avctx, AV_LOG_WARNING, "Invalid color transfer 
characteristics %d, setting to unspecified\n", s->avctx->color_trc);
+s->avctx->color_trc = AVCOL_TRC_UNSPECIFIED;
+}
 s->avctx->colorspace  = get_bits(>gb, 8);
 if (!av_color_space_name(s->avctx->colorspace)) {
 av_log(s->avctx, AV_LOG_WARNING, "Invalid color space %d, setting 
to unspecified\n", s->avctx->colorspace);
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/3] mpeg12dec: validate color space

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavcodec/mpeg12dec.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 63979079c8..d3dc67ad6a 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -1470,6 +1470,10 @@ static void 
mpeg_decode_sequence_display_extension(Mpeg1Context *s1)
 s->avctx->color_primaries = get_bits(>gb, 8);
 s->avctx->color_trc   = get_bits(>gb, 8);
 s->avctx->colorspace  = get_bits(>gb, 8);
+if (!av_color_space_name(s->avctx->colorspace)) {
+av_log(s->avctx, AV_LOG_WARNING, "Invalid color space %d, setting 
to unspecified\n", s->avctx->colorspace);
+s->avctx->colorspace = AVCOL_SPC_UNSPECIFIED;
+}
 }
 w = get_bits(>gb, 14);
 skip_bits(>gb, 1); // marker
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/7] avutil: add FF_BAIL_ON_OVERFLOW

[Andreas Cadhalpun]

On 21.12.2016 13:46, wm4 wrote:
> On Wed, 21 Dec 2016 01:43:46 +0100
> Andreas Cadhalpun <andreas.cadhal...@googlemail.com> wrote:
>> On 20.12.2016 15:22, wm4 wrote:
>>> On Mon, 19 Dec 2016 23:36:11 +0100
>>> Andreas Cadhalpun <andreas.cadhal...@googlemail.com> wrote:
>>>> On 16.12.2016 17:22, wm4 wrote:  
>>>>> Are you sure we need the message?
>>>>
>>>> Yes, since such an overflow could just be a sign of a limitation in our
>>>> framework (think of bit_rate being int32_t) and does not necessarily mean
>>>> that the sample is invalid.
>>>>  
>>>>> It's quite ugly.
>>>>
>>>> Do you have any suggestions for improving it?  
>>>
>>> I'm pretty much against such macros for rather specific use-cases, and
>>> putting them into a public headers.  
>>
>> It is added to an "internal and external API header".
>> Feel free to send patches separating it into an internal and a public header.
> 
> Macros starting with FF are public API,

No, public macros start with AV.

> so don't put that macro in a public header. Or we're stuck with it forever.
> 
>>> I'm thinking it'd be better to actually provide overflow-checking 
>>> primitives,  
>>
>> Why?
> 
> Because that would have actual value,

That's a meaningless argument.

> since overflowing checks are annoying

Using overflow-checking primitives is even more annoying.

> and there's also a chance to get them wrong.

That's always the case with anything.

> The code gets less ugly too.

Rather the contrary. Compare
FF_RETURN_ON_OVERFLOW(s, ea->bytes > INT_MAX / 8 / 2)
st->codecpar->bits_per_coded_sample = ea->bytes * 8;
st->codecpar->bit_rate  = (int64_t)st->codecpar->channels *
  st->codecpar->sample_rate *
  
st->codecpar->bits_per_coded_sample / 4;
st->codecpar->block_align   = st->codecpar->channels *
  
st->codecpar->bits_per_coded_sample;
with:
ret = ff_mul_check_overflow(>codecpar->bits_per_coded_sample, 
ea->bytes, 8)
if (ret < 0)
return ret;
st->codecpar->bit_rate  = (int64_t)st->codecpar->channels *
  st->codecpar->sample_rate *
  
st->codecpar->bits_per_coded_sample / 4;
ret = ff_mul_check_overflow(>codecpar->block_align, 
st->codecpar->channels, st->codecpar->bits_per_coded_sample)
if (ret < 0)
return ret;

> If you're going to add such overflow checks to every
> single operation in libavcodec that could overflow, you better come up
> with something that won't add tons of crap to the code.

Code that overflows is "crap", not the checks preventing that.

>>> and I also don't think every overflow has to be logged.  
>>
>> I disagree for the reason I mentioned above.
> 
> Which was? Not going to read the whole thread again.

Reading either of the mails you replied to would have been sufficient.
You've got a third chance with this mail.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/6] lavfi/buffersink: add accessors for the stream properties.

[Andreas Cadhalpun]

On 20.12.2016 15:46, wm4 wrote:
> In general, replacing public fields with macro-generated accessors is
> really just a rename. Admittedly, it removes a confusing indirection
> (though ->inputs[0]) in this case, but in general the improvements are
> very minor. What does it matter if whether there are 100 fields or 100
> set/get functions?

There are several benefits of using accessors:
 * It is much easier to keep the ABI stable for accessor functions than
   for public structs, because the order of members doesn't affect them.
 * It is much easier to check which binary uses which ABI, because the
   functions are listed in the symbols table, while checking which
   struct member is accessed requires disassembling.
 * Having the struct private means it can't be allocated on the stack
   by API users, preventing problems when the struct size changes.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/7] avutil: add FF_BAIL_ON_OVERFLOW

[Andreas Cadhalpun]

On 20.12.2016 15:22, wm4 wrote:
> On Mon, 19 Dec 2016 23:36:11 +0100
> Andreas Cadhalpun <andreas.cadhal...@googlemail.com> wrote:
> 
>> On 16.12.2016 17:22, wm4 wrote:
>>> On Fri, 16 Dec 2016 03:32:07 +0100
>>> Andreas Cadhalpun <andreas.cadhal...@googlemail.com> wrote:
>>>   
>>>> Suggested-by: Rodger Combs <rodger.co...@gmail.com>
>>>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>>>> ---
>>>>  libavutil/common.h | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/libavutil/common.h b/libavutil/common.h
>>>> index 8142b31..00b7504 100644
>>>> --- a/libavutil/common.h
>>>> +++ b/libavutil/common.h
>>>> @@ -99,6 +99,8 @@
>>>>  #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)
>>>>  #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
>>>>  
>>>> +#define FF_BAIL_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR, 
>>>> "Overflow check failed: " #x"\n"); return AVERROR_INVALIDDATA;}
>>>> +
>>>>  /* misc math functions */
>>>>  
>>>>  #ifdef HAVE_AV_CONFIG_H  
>>>
>>> Are you sure we need the message?  
>>
>> Yes, since such an overflow could just be a sign of a limitation in our
>> framework (think of bit_rate being int32_t) and does not necessarily mean
>> that the sample is invalid.
>>
>>> It's quite ugly.  
>>
>> Do you have any suggestions for improving it?
> 
> I'm pretty much against such macros for rather specific use-cases, and
> putting them into a public headers.

It is added to an "internal and external API header".
Feel free to send patches separating it into an internal and a public header.

> I'm thinking it'd be better to actually provide overflow-checking primitives,

Why?

> and I also don't think every overflow has to be logged.

I disagree for the reason I mentioned above.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/7] avutil: add FF_BAIL_ON_OVERFLOW

[Andreas Cadhalpun]

On 20.12.2016 10:23, Muhammad Faiz wrote:
> On 12/20/16, Andreas Cadhalpun <andreas.cadhal...@googlemail.com> wrote:
>> On 16.12.2016 07:36, Muhammad Faiz wrote:
>>> On 12/16/16, Andreas Cadhalpun <andreas.cadhal...@googlemail.com> wrote:
>>>> Suggested-by: Rodger Combs <rodger.co...@gmail.com>
>>>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>>>> ---
>>>>  libavutil/common.h | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/libavutil/common.h b/libavutil/common.h
>>>> index 8142b31..00b7504 100644
>>>> --- a/libavutil/common.h
>>>> +++ b/libavutil/common.h
>>>> @@ -99,6 +99,8 @@
>>>>  #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a=
>>>> SWAP_tmp;}while(0)
>>>>  #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
>>>>
>>>> +#define FF_BAIL_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR,
>>>> "Overflow check failed: " #x"\n"); return AVERROR_INVALIDDATA;}
>>>
>>> Where is the overflow check calculation?
>>
>> The parameter 'x' is the overflow check used in 'if (x)'.
> 
> Is it impossible to do something like
> int ff_mul_check_overflow(int *result, int a, int b)
> with AVERROR return code on overlow?

Not really, as the point of the macro is to do the error handling,
which would be needed for a function. Also the function is not
generic enough, as the type can be int64_t. And then using such
a function would make the code rather less readable.

> I suggest this is AVERROR(ERANGE)

This seems like a better fit for the error type, so I've locally
change the error in the macro to this.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] swscale: also save ebx register when using PIE

[Andreas Cadhalpun]

On 20.12.2016 03:04, Michael Niedermayer wrote:
> On Mon, Dec 19, 2016 at 11:28:44PM +0100, Andreas Cadhalpun wrote:
>> On 16.12.2016 04:08, Michael Niedermayer wrote:
>>> On Fri, Dec 16, 2016 at 02:36:53AM +0100, Andreas Cadhalpun wrote:
>>>> Otherwise the build fails when configuring with --toolchain=hardened
>>>> --disable-pic on i386 using gcc 4.8:
>>>> error: PIC register clobbered by '%ebx' in 'asm'
>>>>
>>>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>>>> ---
>>>>  libswscale/x86/hscale_fast_bilinear_simd.c | 20 ++--
>>>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/libswscale/x86/hscale_fast_bilinear_simd.c 
>>>> b/libswscale/x86/hscale_fast_bilinear_simd.c
>>>> index 2cba5f0..3f0f5f5 100644
>>>> --- a/libswscale/x86/hscale_fast_bilinear_simd.c
>>>> +++ b/libswscale/x86/hscale_fast_bilinear_simd.c
>>>> @@ -199,7 +199,7 @@ void ff_hyscale_fast_mmxext(SwsContext *c, int16_t 
>>>> *dst,
>>>>  #if ARCH_X86_64
>>>>  uint64_t retsave;
>>>>  #else
>>>> -#if defined(PIC)
>>>> +#if defined(PIC) || defined(__PIE__)
>>>
>>> please correct me if iam wrong
>>> our configure adds -DPIC to define PIC when its enabled,
>>> it does not add that in this case but gcc is still generating PIC code
>>> that doesnt seem good
>>
>> gcc does not generate PIC, only PIE, which is subtly different.
> 
> does all the code under PIC work with
> PIE that does not have PIC set ?

It seems so, as the full FATE test passes.

> the identifier seems used a bit in .asm files
> 
>> What's wrong here is that this code in swscale tries to determine, whether
>> or not the ebx register can be used for asm, but doesn't check that 
>> correctly.
>> However, configure has a working check for that, the result of which can
>> be used here. Patch doing that is attached.
> 
> i see your argument for this and it seems sound.
> I hope this doesnt break anything as this logic was that way for a
> really long time and worked fine and gcc inline asm can be annoying

I think the regression potential is rather low, as current gcc does not require
saving the ebx register anymore.

> that said, no objections to the patch 

Pushed.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/6] lavfi/buffersink: add accessors for the stream properties.

[Andreas Cadhalpun]

On 18.12.2016 13:22, Nicolas George wrote:
> av_buffersink_get_frame_rate() did already exist; its argument becomes const.
> 
> TODO minor version bump
> 
> API-Change: libavfilter
> Signed-off-by: Nicolas George 
> ---
>  libavfilter/buffersink.c | 25 +++--
>  libavfilter/buffersink.h | 22 --
>  2 files changed, 39 insertions(+), 8 deletions(-)
> 
> 
> I think the const change is acceptable.

I'm not aware of a user outside of ffmpeg and const changes shouldn't cause big
problems, as only the API changes, not the ABI. So it is probably OK.

> Note: I am introducing the "API-Change" Git tag: I think it will be much
> more convenient than maintaining doc/APIchanges. Later I intend to write a
> script using git log --grep to pretty print it.

I'm not convinced that this is more convenient. APIchanges can still be
changed after a commit, but the commit message can't. Also it can only replace
APIchanges when everyone (including Libav) uses it.
So I'd prefer if you added an APIchanges entry in addition to using this tag.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] wmavoice: don't error out if we're skipping more bits than available.

[Andreas Cadhalpun]

On 20.12.2016 00:59, Carl Eugen Hoyos wrote:
> 2016-12-19 23:45 GMT+01:00 Andreas Cadhalpun 
> <andreas.cadhal...@googlemail.com>:
>> On 16.12.2016 14:19, Ronald S. Bultje wrote:
>>> This reverts 2a4700a4f03280fa8ba4fc0f8a9987bb550f0d1e and implements it
>>> correctly so streams actually decode the way the encoder intended them
>>> to.
>>
>> Why is it correct?
> 
> Because the patch introduced an (imo horrible) regression,

I don't think that not triggering an av_assert0 is a regression.

> files that before reported why they couldn't be played suddenly
> gave no explanation, just a useless error message.

An error message is not no explanation.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] wmavoice: don't error out if we're skipping more bits than available.

[Andreas Cadhalpun]

On 16.12.2016 14:19, Ronald S. Bultje wrote:
> This reverts 2a4700a4f03280fa8ba4fc0f8a9987bb550f0d1e and implements it
> correctly so streams actually decode the way the encoder intended them
> to.

Why is it correct?
Is this behavior defined in a specification or reference decoder?

> ---
>  libavcodec/wmavoice.c | 20 
>  1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
> index 0f29bdd..f1b5369 100644
> --- a/libavcodec/wmavoice.c
> +++ b/libavcodec/wmavoice.c
> @@ -1900,16 +1900,10 @@ static int wmavoice_decode_packet(AVCodecContext 
> *ctx, void *data,
>  cnt += s->spillover_nbits;
>  s->skip_bits_next = cnt & 7;
>  res = cnt >> 3;
> -if (res > avpkt->size) {
> -av_log(ctx, AV_LOG_ERROR,
> -   "Trying to skip %d bytes in packet of size 
> %d\n",
> -   res, avpkt->size);
> -return AVERROR_INVALIDDATA;
> -}
> -return res;
> +return FFMIN(avpkt->size, res);
>  } else
> -skip_bits_long (gb, s->spillover_nbits - cnt +
> -get_bits_count(gb)); // resync
> +skip_bits_long(gb, s->spillover_nbits - cnt +
> +   get_bits_count(gb)); // resync

Please do cosmetic changes in a separate commit.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/7] avutil: add FF_BAIL_ON_OVERFLOW

[Andreas Cadhalpun]

On 16.12.2016 17:22, wm4 wrote:
> On Fri, 16 Dec 2016 03:32:07 +0100
> Andreas Cadhalpun <andreas.cadhal...@googlemail.com> wrote:
> 
>> Suggested-by: Rodger Combs <rodger.co...@gmail.com>
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavutil/common.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/libavutil/common.h b/libavutil/common.h
>> index 8142b31..00b7504 100644
>> --- a/libavutil/common.h
>> +++ b/libavutil/common.h
>> @@ -99,6 +99,8 @@
>>  #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)
>>  #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
>>  
>> +#define FF_BAIL_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR, 
>> "Overflow check failed: " #x"\n"); return AVERROR_INVALIDDATA;}
>> +
>>  /* misc math functions */
>>  
>>  #ifdef HAVE_AV_CONFIG_H
> 
> Are you sure we need the message?

Yes, since such an overflow could just be a sign of a limitation in our
framework (think of bit_rate being int32_t) and does not necessarily mean
that the sample is invalid.

> It's quite ugly.

Do you have any suggestions for improving it?

> Also maybe call it "FF_RETURN_ON_OVERFLOW".

That sounds a bit better, so changed locally.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] swscale: also save ebx register when using PIE

[Andreas Cadhalpun]

On 16.12.2016 04:08, Michael Niedermayer wrote:
> On Fri, Dec 16, 2016 at 02:36:53AM +0100, Andreas Cadhalpun wrote:
>> Otherwise the build fails when configuring with --toolchain=hardened
>> --disable-pic on i386 using gcc 4.8:
>> error: PIC register clobbered by '%ebx' in 'asm'
>>
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libswscale/x86/hscale_fast_bilinear_simd.c | 20 ++--
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/libswscale/x86/hscale_fast_bilinear_simd.c 
>> b/libswscale/x86/hscale_fast_bilinear_simd.c
>> index 2cba5f0..3f0f5f5 100644
>> --- a/libswscale/x86/hscale_fast_bilinear_simd.c
>> +++ b/libswscale/x86/hscale_fast_bilinear_simd.c
>> @@ -199,7 +199,7 @@ void ff_hyscale_fast_mmxext(SwsContext *c, int16_t *dst,
>>  #if ARCH_X86_64
>>  uint64_t retsave;
>>  #else
>> -#if defined(PIC)
>> +#if defined(PIC) || defined(__PIE__)
> 
> please correct me if iam wrong
> our configure adds -DPIC to define PIC when its enabled,
> it does not add that in this case but gcc is still generating PIC code
> that doesnt seem good

gcc does not generate PIC, only PIE, which is subtly different.

What's wrong here is that this code in swscale tries to determine, whether
or not the ebx register can be used for asm, but doesn't check that correctly.
However, configure has a working check for that, the result of which can
be used here. Patch doing that is attached.

Best regards,
Andreas
>From 7b5d0714482a0ec42d317fa1e9fa095180e39ccd Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
Date: Fri, 16 Dec 2016 02:29:56 +0100
Subject: [PATCH] swscale: save ebx register when it is not available

Configure checks if the ebx register can be used for asm and it has to
be saved if and only if this is not the case.
Without this the build fails when configuring with --toolchain=hardened
--disable-pic on i386 using gcc 4.8:
error: PIC register clobbered by '%ebx' in 'asm'

In that case gcc 4.8 reserves the ebx register for the GOT needed for
PIE, so it can't be used in asm directly.

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libswscale/x86/hscale_fast_bilinear_simd.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/libswscale/x86/hscale_fast_bilinear_simd.c b/libswscale/x86/hscale_fast_bilinear_simd.c
index 2cba5f0a1c..60a2cbfc50 100644
--- a/libswscale/x86/hscale_fast_bilinear_simd.c
+++ b/libswscale/x86/hscale_fast_bilinear_simd.c
@@ -199,7 +199,7 @@ void ff_hyscale_fast_mmxext(SwsContext *c, int16_t *dst,
 #if ARCH_X86_64
 uint64_t retsave;
 #else
-#if defined(PIC)
+#if !HAVE_EBX_AVAILABLE
 uint64_t ebxsave;
 #endif
 #endif
@@ -209,7 +209,7 @@ void ff_hyscale_fast_mmxext(SwsContext *c, int16_t *dst,
 "mov   -8(%%rsp), %%"FF_REG_a"\n\t"
 "mov%%"FF_REG_a", %5  \n\t"  // retsave
 #else
-#if defined(PIC)
+#if !HAVE_EBX_AVAILABLE
 "mov%%"FF_REG_b", %5  \n\t"  // ebxsave
 #endif
 #endif
@@ -255,7 +255,7 @@ void ff_hyscale_fast_mmxext(SwsContext *c, int16_t *dst,
 "mov  %5, %%"FF_REG_a" \n\t"
 "mov%%"FF_REG_a", -8(%%rsp)\n\t"
 #else
-#if defined(PIC)
+#if !HAVE_EBX_AVAILABLE
 "mov  %5, %%"FF_REG_b" \n\t"
 #endif
 #endif
@@ -264,12 +264,12 @@ void ff_hyscale_fast_mmxext(SwsContext *c, int16_t *dst,
 #if ARCH_X86_64
   ,"m"(retsave)
 #else
-#if defined(PIC)
+#if !HAVE_EBX_AVAILABLE
   ,"m" (ebxsave)
 #endif
 #endif
 : "%"FF_REG_a, "%"FF_REG_c, "%"FF_REG_d, "%"FF_REG_S, "%"FF_REG_D
-#if ARCH_X86_64 || !defined(PIC)
+#if ARCH_X86_64 || HAVE_EBX_AVAILABLE
  ,"%"FF_REG_b
 #endif
 );
@@ -289,7 +289,7 @@ void ff_hcscale_fast_mmxext(SwsContext *c, int16_t *dst1, int16_t *dst2,
 #if ARCH_X86_64
 DECLARE_ALIGNED(8, uint64_t, retsave);
 #else
-#if defined(PIC)
+#if !HAVE_EBX_AVAILABLE
 DECLARE_ALIGNED(8, uint64_t, ebxsave);
 #endif
 #endif
@@ -298,7 +298,7 @@ void ff_hcscale_fast_mmxext(SwsContext *c, int16_t *dst1, int16_t *dst2,
 "mov  -8(%%rsp), %%"FF_REG_a"\n\t"
 "mov   %%"FF_REG_a", %7  \n\t"  // retsave
 #else
-#if defined(PIC)
+#if !HAVE_EBX_AVAILABLE
 "mov   %%"FF_REG_b", %7  \n\t"  // ebxsave
 #endif
 #endif
@@ -332,7 +332,7 @@ void ff_hcscale_fast_mmxext(SwsContext *c, int16_t *dst1, int16_t *dst2,
 "mov%7, %%"

Re: [FFmpeg-devel] [PATCH 1/7] avutil: add FF_BAIL_ON_OVERFLOW

[Andreas Cadhalpun]

On 16.12.2016 07:36, Muhammad Faiz wrote:
> On 12/16/16, Andreas Cadhalpun <andreas.cadhal...@googlemail.com> wrote:
>> Suggested-by: Rodger Combs <rodger.co...@gmail.com>
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>>  libavutil/common.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/libavutil/common.h b/libavutil/common.h
>> index 8142b31..00b7504 100644
>> --- a/libavutil/common.h
>> +++ b/libavutil/common.h
>> @@ -99,6 +99,8 @@
>>  #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)
>>  #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
>>
>> +#define FF_BAIL_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR,
>> "Overflow check failed: " #x"\n"); return AVERROR_INVALIDDATA;}
> 
> Where is the overflow check calculation?

The parameter 'x' is the overflow check used in 'if (x)'.

> What about functions that need clean up with goto before return?

This is only needed rarely, e.g. in none of the patches I sent.
It happens occasionally for the more common checks needed to
validate codec parameters that I'm working on, but these can be
handled on a case by case basis.
The general macros are only for the common, trivial cases.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/7] 4xm: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 16.12.2016 03:33, Rodger Combs wrote:
> 
>> On Dec 15, 2016, at 20:32, Andreas Cadhalpun 
>> <andreas.cadhal...@googlemail.com> wrote:
>>
>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
>> ---
>> libavformat/4xm.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/libavformat/4xm.c b/libavformat/4xm.c
>> index 2758b69..9332f78 100644
>> --- a/libavformat/4xm.c
>> +++ b/libavformat/4xm.c
>> @@ -187,6 +187,7 @@ static int parse_strk(AVFormatContext *s,
>> st->codecpar->bit_rate  = (int64_t)st->codecpar->channels *
>>   st->codecpar->sample_rate *
>>   
>> st->codecpar->bits_per_coded_sample;
>> +FF_BAIL_ON_OVERFLOW(s, st->codecpar->channels && 
>> st->codecpar->bits_per_coded_sample > INT_MAX / st->codecpar->channels)
> 
> Shouldn't this go before the actual (potentially-overflowing) calculation is 
> done?

It is.

>> st->codecpar->block_align   = st->codecpar->channels *
>>   
>> st->codecpar->bits_per_coded_sample;

This here is the problem.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 6/7] nistspheredec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/nistspheredec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c
index 782d1df..b5ebcb8 100644
--- a/libavformat/nistspheredec.c
+++ b/libavformat/nistspheredec.c
@@ -80,6 +80,7 @@ static int nist_read_header(AVFormatContext *s)
 
 avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
 
+FF_BAIL_ON_OVERFLOW(s, st->codecpar->channels && 
st->codecpar->bits_per_coded_sample > INT_MAX / st->codecpar->channels)
 st->codecpar->block_align = st->codecpar->bits_per_coded_sample * 
st->codecpar->channels / 8;
 
 if (avio_tell(s->pb) > header_size)
-- 
2.10.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 7/7] pvfdec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/pvfdec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/pvfdec.c b/libavformat/pvfdec.c
index b9f6d4f..868ef0b 100644
--- a/libavformat/pvfdec.c
+++ b/libavformat/pvfdec.c
@@ -56,6 +56,7 @@ static int pvf_read_header(AVFormatContext *s)
 st->codecpar->sample_rate = sample_rate;
 st->codecpar->codec_id= ff_get_pcm_codec_id(bps, 0, 1, 0x);
 st->codecpar->bits_per_coded_sample = bps;
+FF_BAIL_ON_OVERFLOW(s, bps > INT_MAX / st->codecpar->channels)
 st->codecpar->block_align = bps * st->codecpar->channels / 8;
 
 avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
-- 
2.10.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 4/7] genh: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/genh.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/genh.c b/libavformat/genh.c
index b683e02..e23c1b2 100644
--- a/libavformat/genh.c
+++ b/libavformat/genh.c
@@ -74,6 +74,7 @@ static int genh_read_header(AVFormatContext *s)
 case  0: st->codecpar->codec_id = AV_CODEC_ID_ADPCM_PSX;break;
 case  1:
 case 11: st->codecpar->bits_per_coded_sample = 4;
+ FF_BAIL_ON_OVERFLOW(s, st->codecpar->channels > INT_MAX / 36)
  st->codecpar->block_align = 36 * st->codecpar->channels;
  st->codecpar->codec_id = AV_CODEC_ID_ADPCM_IMA_WAV;break;
 case  2: st->codecpar->codec_id = AV_CODEC_ID_ADPCM_DTK;break;
-- 
2.10.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 5/7] ircamdec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/ircamdec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/ircamdec.c b/libavformat/ircamdec.c
index 59f3a49..3f92b6c 100644
--- a/libavformat/ircamdec.c
+++ b/libavformat/ircamdec.c
@@ -96,6 +96,7 @@ static int ircam_read_header(AVFormatContext *s)
 }
 
 st->codecpar->bits_per_coded_sample = 
av_get_bits_per_sample(st->codecpar->codec_id);
+FF_BAIL_ON_OVERFLOW(s, st->codecpar->channels && 
st->codecpar->bits_per_coded_sample > INT_MAX / st->codecpar->channels)
 st->codecpar->block_align = st->codecpar->bits_per_coded_sample * 
st->codecpar->channels / 8;
 avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
 avio_skip(s->pb, 1008);
-- 
2.10.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 3/7] electronicarts: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/electronicarts.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/electronicarts.c b/libavformat/electronicarts.c
index 30eb723..9088fe1 100644
--- a/libavformat/electronicarts.c
+++ b/libavformat/electronicarts.c
@@ -556,6 +556,7 @@ static int ea_read_header(AVFormatContext *s)
 st->codecpar->codec_tag = 0;   /* no tag */
 st->codecpar->channels  = ea->num_channels;
 st->codecpar->sample_rate   = ea->sample_rate;
+FF_BAIL_ON_OVERFLOW(s, ea->bytes > INT_MAX / 8 / 2)
 st->codecpar->bits_per_coded_sample = ea->bytes * 8;
 st->codecpar->bit_rate  = (int64_t)st->codecpar->channels *
   st->codecpar->sample_rate *
-- 
2.10.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/7] 4xm: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavformat/4xm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/4xm.c b/libavformat/4xm.c
index 2758b69..9332f78 100644
--- a/libavformat/4xm.c
+++ b/libavformat/4xm.c
@@ -187,6 +187,7 @@ static int parse_strk(AVFormatContext *s,
 st->codecpar->bit_rate  = (int64_t)st->codecpar->channels *
   st->codecpar->sample_rate *
   st->codecpar->bits_per_coded_sample;
+FF_BAIL_ON_OVERFLOW(s, st->codecpar->channels && 
st->codecpar->bits_per_coded_sample > INT_MAX / st->codecpar->channels)
 st->codecpar->block_align   = st->codecpar->channels *
   st->codecpar->bits_per_coded_sample;
 
-- 
2.10.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/7] avutil: add FF_BAIL_ON_OVERFLOW

[Andreas Cadhalpun]

Suggested-by: Rodger Combs <rodger.co...@gmail.com>
Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libavutil/common.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavutil/common.h b/libavutil/common.h
index 8142b31..00b7504 100644
--- a/libavutil/common.h
+++ b/libavutil/common.h
@@ -99,6 +99,8 @@
 #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)
 #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
 
+#define FF_BAIL_ON_OVERFLOW(ctx, x) if (x) {av_log(ctx, AV_LOG_ERROR, 
"Overflow check failed: " #x"\n"); return AVERROR_INVALIDDATA;}
+
 /* misc math functions */
 
 #ifdef HAVE_AV_CONFIG_H
-- 
2.10.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] 4xm: prevent overflow during bit rate calculation

[Andreas Cadhalpun]

On 16.12.2016 02:29, Rodger Combs wrote: 
>> On Dec 15, 2016, at 19:21, Andreas Cadhalpun 
>> <andreas.cadhal...@googlemail.com> wrote:
>> On 15.12.2016 14:02, Ronald S. Bultje wrote:
>>> - if for whatever reason some things cannot be done in generic code or by
>>> changing the type (this should really cover most cases), and we want
>>> specific overflow checks, then maybe we want to have some generic helper
>>> macros that make them one-liners in decoders. This would return an error
>>> along with fixing the UB.
>>
>> I don't think the number of overflow checks added justifies the additional
>> complexity of factoring things out. These checks are also subtly different,
>> so it's not easy to write a generic helper for that.
>> However, I plan to do this for the actually common cases when validating
>> codec parameters, like checking that a parameter is not negative.
>>
> 
> My proposal was for something like:
> #define BAIL_ON_OVERFLOW(x) if (x) {av_log(avctx, AV_LOG_ERROR, "Overflow 
> check failed: " #x); return AVERROR_INVALIDDATA;}
> Which basically reduces the code overhead down to a simple one-liner.

Yeah, that's similar to how I plan to handle the more common cases.

> It's hard to get detailed error prints out of this, but if we're saying these
> cases are so unlikely (fuzzer-only?) that we're comfortable outright failing
> on them, the level of precision in the message probably doesn't matter much?

Agreed, so I've updated the patch series using this approach.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] swscale: also save ebx register when using PIE

[Andreas Cadhalpun]

Otherwise the build fails when configuring with --toolchain=hardened
--disable-pic on i386 using gcc 4.8:
error: PIC register clobbered by '%ebx' in 'asm'

Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
 libswscale/x86/hscale_fast_bilinear_simd.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/libswscale/x86/hscale_fast_bilinear_simd.c 
b/libswscale/x86/hscale_fast_bilinear_simd.c
index 2cba5f0..3f0f5f5 100644
--- a/libswscale/x86/hscale_fast_bilinear_simd.c
+++ b/libswscale/x86/hscale_fast_bilinear_simd.c
@@ -199,7 +199,7 @@ void ff_hyscale_fast_mmxext(SwsContext *c, int16_t *dst,
 #if ARCH_X86_64
 uint64_t retsave;
 #else
-#if defined(PIC)
+#if defined(PIC) || defined(__PIE__)
 uint64_t ebxsave;
 #endif
 #endif
@@ -209,7 +209,7 @@ void ff_hyscale_fast_mmxext(SwsContext *c, int16_t *dst,
 "mov   -8(%%rsp), %%"FF_REG_a"\n\t"
 "mov%%"FF_REG_a", %5  \n\t"  // retsave
 #else
-#if defined(PIC)
+#if defined(PIC) || defined(__PIE__)
 "mov%%"FF_REG_b", %5  \n\t"  // ebxsave
 #endif
 #endif
@@ -255,7 +255,7 @@ void ff_hyscale_fast_mmxext(SwsContext *c, int16_t *dst,
 "mov  %5, %%"FF_REG_a" \n\t"
 "mov%%"FF_REG_a", -8(%%rsp)\n\t"
 #else
-#if defined(PIC)
+#if defined(PIC) || defined(__PIE__)
 "mov  %5, %%"FF_REG_b" \n\t"
 #endif
 #endif
@@ -264,12 +264,12 @@ void ff_hyscale_fast_mmxext(SwsContext *c, int16_t *dst,
 #if ARCH_X86_64
   ,"m"(retsave)
 #else
-#if defined(PIC)
+#if defined(PIC) || defined(__PIE__)
   ,"m" (ebxsave)
 #endif
 #endif
 : "%"FF_REG_a, "%"FF_REG_c, "%"FF_REG_d, "%"FF_REG_S, "%"FF_REG_D
-#if ARCH_X86_64 || !defined(PIC)
+#if ARCH_X86_64 || !(defined(PIC) || defined(__PIE__))
  ,"%"FF_REG_b
 #endif
 );
@@ -289,7 +289,7 @@ void ff_hcscale_fast_mmxext(SwsContext *c, int16_t *dst1, 
int16_t *dst2,
 #if ARCH_X86_64
 DECLARE_ALIGNED(8, uint64_t, retsave);
 #else
-#if defined(PIC)
+#if defined(PIC) || defined(__PIE__)
 DECLARE_ALIGNED(8, uint64_t, ebxsave);
 #endif
 #endif
@@ -298,7 +298,7 @@ void ff_hcscale_fast_mmxext(SwsContext *c, int16_t *dst1, 
int16_t *dst2,
 "mov  -8(%%rsp), %%"FF_REG_a"\n\t"
 "mov   %%"FF_REG_a", %7  \n\t"  // retsave
 #else
-#if defined(PIC)
+#if defined(PIC) || defined(__PIE__)
 "mov   %%"FF_REG_b", %7  \n\t"  // ebxsave
 #endif
 #endif
@@ -332,7 +332,7 @@ void ff_hcscale_fast_mmxext(SwsContext *c, int16_t *dst1, 
int16_t *dst2,
 "mov%7, %%"FF_REG_a" \n\t"
 "mov  %%"FF_REG_a", -8(%%rsp)\n\t"
 #else
-#if defined(PIC)
+#if defined(PIC) || defined(__PIE__)
 "mov %7, %%"FF_REG_b"\n\t"
 #endif
 #endif
@@ -341,12 +341,12 @@ void ff_hcscale_fast_mmxext(SwsContext *c, int16_t *dst1, 
int16_t *dst2,
 #if ARCH_X86_64
   ,"m"(retsave)
 #else
-#if defined(PIC)
+#if defined(PIC) || defined(__PIE__)
   ,"m" (ebxsave)
 #endif
 #endif
 : "%"FF_REG_a, "%"FF_REG_c, "%"FF_REG_d, "%"FF_REG_S, "%"FF_REG_D
-#if ARCH_X86_64 || !defined(PIC)
+#if ARCH_X86_64 || !(defined(PIC) || defined(__PIE__))
  ,"%"FF_REG_b
 #endif
 );
-- 
2.10.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavf/mov.c: Avoid heap allocation wrap in mov_read_hdlr

[Andreas Cadhalpun]

On 15.12.2016 03:25, James Almer wrote:
> On 12/14/2016 10:39 PM, Andreas Cadhalpun wrote:
>> On 15.12.2016 00:34, Matthew Wolenetz wrote:
>>>
>>> From fd878457cd55690d4a27d74411b68a30c9fb2313 Mon Sep 17 00:00:00 2001
>>> From: Matt Wolenetz <wolen...@chromium.org>
>>> Date: Fri, 2 Dec 2016 18:10:39 -0800
>>> Subject: [PATCH] lavf/mov.c: Avoid heap allocation wrap in mov_read_hdlr
>>>
>>> Core of patch is from p...@paulmehta.com
>>> Reference https://crbug.com/643950
>>> ---
>>>  libavformat/mov.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>> index 2a69890..7254505 100644
>>> --- a/libavformat/mov.c
>>> +++ b/libavformat/mov.c
>>> @@ -739,6 +739,8 @@ static int mov_read_hdlr(MOVContext *c, AVIOContext 
>>> *pb, MOVAtom atom)
>>>  
>>>  title_size = atom.size - 24;
>>>  if (title_size > 0) {
>>> +if (title_size >= UINT_MAX)
>>
>> I think this should use SIZE_MAX.
> 
> title_size is int64_t and SIZE_MAX is UINT64_MAX on x86_64.

Yes, but the argument of av_malloc is size_t.

>>
>>> +return AVERROR_INVALIDDATA;
>>>  title_str = av_malloc(title_size + 1); /* Add null terminator */

So this should cast the argument to size_t to fix the issue on x86_64:
  title_str = av_malloc((size_t)title_size + 1); /* Add null terminator 
*/

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] 4xm: prevent overflow during bit rate calculation

[Andreas Cadhalpun]

On 15.12.2016 14:02, Ronald S. Bultje wrote:
> On Wed, Dec 14, 2016 at 7:11 PM, Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com> wrote:
>> On 14.12.2016 02:46, Ronald S. Bultje wrote:
>>> Not wanting to discourage you, but I wonder if there's really a point to
>>> this...?
>>
>> These patches are prerequisites for enforcing the validity of codec
>> parameters [1].
>>
>>> I don't see how the user experience changes.
>>
>> Users won't see ffmpeg claiming nonsense bit rates like -1184293205235990
>> kb/s
>> anymore.
> 
> 
> I don't think you understand my question.

Maybe you just didn't understand my answer.

> - does this belong in 4xm.c? (Or in generic code? Or in the app?)

I think it belongs both in 4xm.c and generic code, as I have explained
in detail [1] in the discussion back then.

> - should it return an error? (Or just clip parameters? Or ignore the
> invalid value?)

This was also already discussed [2].

On 15.12.2016 21:57, Ronald S. Bultje wrote:
> So, I asked on IRC, here's 3 suggestions from Roger Combs:
> 
> - in case we want to be pedantic (I personally don't care, but I understand
> other people are), it might make sense to just make these members
> (channels, block_align, bitrate) unsigned. That removes the UB of the
> overflow, and it fixes the negative number reporting in client apps for
> bitrate, but you can still have positive crazy numbers and it doesn't
> return an error.

These are public fields, so changing them is not easy and more importantly
changing them from signed to unsigned is a very bad idea, as it will most
probably cause all kinds of subtle bugs for API users, e.g. when comparing,
subtracting, etc. and not expecting unsigned behavior.

> - if for whatever reason some things cannot be done in generic code or by
> changing the type (this should really cover most cases), and we want
> specific overflow checks, then maybe we want to have some generic helper
> macros that make them one-liners in decoders. This would return an error
> along with fixing the UB.

I don't think the number of overflow checks added justifies the additional
complexity of factoring things out. These checks are also subtly different,
so it's not easy to write a generic helper for that.
However, I plan to do this for the actually common cases when validating
codec parameters, like checking that a parameter is not negative.

> - have overflow-safe multiplication functions instead of checking each
> argument before doing a multiply, like __builtin_mul_overflow, and then
> return INT64_MAX if it would overflow inside that function. This fixes
> display of numbers in client applications and the UB, but without returning
> an error.

I think that would be over-engineered.

> What I want most - and this comment applies to all patches of this sort -
> is to have something that we can all agree is OK for pretty much any
> decoder out there without significant overhead in code (source - not
> binary) size. This way, you have a template to work from and fix specific
> instances without us having to argue over every single time you do a next
> run with ubsan.

UBSan is not only about overflows, undefined shifts are also quite common.
But I haven't really looked into these cases yet, so I don't know what kind
of template, if any, would make sense for them.

Best regards,
Andreas


1: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-October/201742.html
2: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-November/203257.html
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavc/libopusdec.c Fix ff_vorbis_channel_layouts OOB

[Andreas Cadhalpun]

On 15.12.2016 00:39, Matthew Wolenetz wrote:
> From 141e56ccf7fc56646424484d357b6c74a486d2e2 Mon Sep 17 00:00:00 2001
> From: Matt Wolenetz 
> Date: Mon, 21 Nov 2016 17:30:50 -0800
> Subject: [PATCH] lavc/libopusdec.c Fix ff_vorbis_channel_layouts OOB
> 
> Similar to existing lavc/vorbisdec.c code which first checks that
> avc->channels is valid for accessing ff_vorbis_channel_layouts, this
> change adds protection to libopusdec.c to prevent accessing that
> array with a negative index. Reference https://crbug.com/666794.
> ---
>  libavcodec/libopusdec.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/libavcodec/libopusdec.c b/libavcodec/libopusdec.c
> index acc62f1..c2c7adc 100644
> --- a/libavcodec/libopusdec.c
> +++ b/libavcodec/libopusdec.c
> @@ -50,6 +50,10 @@ static av_cold int libopus_decode_init(AVCodecContext *avc)
>  avc->sample_rate= 48000;
>  avc->sample_fmt = avc->request_sample_fmt == AV_SAMPLE_FMT_FLT ?
>AV_SAMPLE_FMT_FLT : AV_SAMPLE_FMT_S16;
> +if (avc->channels <= 0) {
> +av_log(avc, AV_LOG_ERROR, "Invalid number of channels\n");
> +return AVERROR(EINVAL);
> +}
>  avc->channel_layout = avc->channels > 8 ? 0 :
>ff_vorbis_channel_layouts[avc->channels - 1];
>  

What version of ffmpeg is this based on?

I'm pretty sure I fixed this issue with commit
8c8f543b81aa2b50bb6a6cfd370a0061281492a3.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavf/mov.c: Avoid heap allocation wraps and OOB in mov_read_{senc, saiz, udta_string}()

[Andreas Cadhalpun]

On 15.12.2016 00:37, Matthew Wolenetz wrote:
> From 8622f9398e7c89a664c4c2ceff9d35b89ff17bb5 Mon Sep 17 00:00:00 2001
> From: Matt Wolenetz 
> Date: Tue, 6 Dec 2016 12:54:23 -0800
> Subject: [PATCH] lavf/mov.c: Avoid heap allocation wraps and OOB in
>  mov_read_{senc,saiz,udta_string}()
> 
> Core of patch is from p...@paulmehta.com
> Reference https://crbug.com/643952
> ---
>  libavformat/mov.c | 11 ---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index e506d20..87ad91a 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -404,7 +404,7 @@ retry:
>  return ret;
>  } else if (!key && c->found_hdlr_mdta && c->meta_keys) {
>  uint32_t index = AV_RB32();
> -if (index < c->meta_keys_count) {
> +if (index < c->meta_keys_count && index > 0) {

This should be in a separate patch.

>  key = c->meta_keys[index];
>  } else {
>  av_log(c->fc, AV_LOG_WARNING,
> @@ -4502,8 +4502,8 @@ static int mov_read_senc(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  
>  avio_rb32(pb);/* entries */
>  
> -if (atom.size < 8) {
> -av_log(c->fc, AV_LOG_ERROR, "senc atom size %"PRId64" too small\n", 
> atom.size);
> +if (atom.size < 8 || atom.size > UINT_MAX) {
> +av_log(c->fc, AV_LOG_ERROR, "senc atom size %"PRId64" invalid\n", 
> atom.size);
>  return AVERROR_INVALIDDATA;
>  }
>  
> @@ -4571,6 +4571,11 @@ static int mov_read_saiz(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  return 0;
>  }
>  
> +if (atom.size > UINT_MAX) {
> +av_log(c->fc, AV_LOG_ERROR, "saiz atom auxiliary_info_sizes size 
> %"PRId64" invalid\n", atom.size);
> +return AVERROR_INVALIDDATA;
> +}
> +
>  /* save the auxiliary info sizes as is */
>  data_size = atom.size - atom_header_size;
>  

And these should also check for SIZE_MAX.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel