audemux: Add a sanity check for the number of channels
diff mbox

Message ID 1335129157-52622-1-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö April 22, 2012, 9:12 p.m. 
From: Michael Niedermayer <michaelni@gmx.at>

Fixes a division by 0.

Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
---
 libavformat/au.c |    5 +++++
 1 file changed, 5 insertions(+)

Comments

Ronald Bultje April 22, 2012, 9:28 p.m. | #1 
Hi,

On Sun, Apr 22, 2012 at 2:12 PM, Martin Storsjö <martin@martin.st> wrote:
> From: Michael Niedermayer <michaelni@gmx.at>
>
> Fixes a division by 0.
>
> Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> ---
>  libavformat/au.c |    5 +++++
>  1 file changed, 5 insertions(+)

LGTM.

Ronald
Mans Rullgard April 22, 2012, 9:35 p.m. | #2 
Martin Storsjö <martin@martin.st> writes:

> From: Michael Niedermayer <michaelni@gmx.at>
>
> Fixes a division by 0.
>
> Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> ---
>  libavformat/au.c |    5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/libavformat/au.c b/libavformat/au.c
> index 3a83d28..3ae784d 100644
> --- a/libavformat/au.c
> +++ b/libavformat/au.c
> @@ -145,6 +145,11 @@ static int au_read_header(AVFormatContext *s)
>          return AVERROR_INVALIDDATA;
>      }
>
> +    if (channels <= 0) {
> +        av_log(s, AV_LOG_ERROR, "Invalid number of channels %d\n", channels);
> +        return AVERROR_INVALIDDATA;
> +    }

channels is declared as unsigned int.  Testing for negative is
pointless, though of course it should not be zero.

It might also make sense to check the sample rate for sanity.
Uoti Urpala April 22, 2012, 9:45 p.m. | #3 
On Mon, 2012-04-23 at 00:12 +0300, Martin Storsjö wrote:
> +    if (channels <= 0) {
> +        av_log(s, AV_LOG_ERROR, "Invalid number of channels %d\n", channels);
> +        return AVERROR_INVALIDDATA;
> +    }

Since the channel count is read with avio_rb32(), it should have a
sanity check for upper limit too. Insanely large channel counts will
cause integer overflows.
Martin Storsjö April 22, 2012, 10:04 p.m. | #4 
On Mon, 23 Apr 2012, Uoti Urpala wrote:

> On Mon, 2012-04-23 at 00:12 +0300, Martin Storsjö wrote:
>> +    if (channels <= 0) {
>> +        av_log(s, AV_LOG_ERROR, "Invalid number of channels %d\n", channels);
>> +        return AVERROR_INVALIDDATA;
>> +    }
>
> Since the channel count is read with avio_rb32(), it should have a
> sanity check for upper limit too. Insanely large channel counts will
> cause integer overflows.

Right. So what would be a sensible limit here, 100 or so?

// Martin
Mans Rullgard April 22, 2012, 10:06 p.m. | #5 
Martin Storsjö <martin@martin.st> writes:

> On Mon, 23 Apr 2012, Uoti Urpala wrote:
>
>> On Mon, 2012-04-23 at 00:12 +0300, Martin Storsjö wrote:
>>> +    if (channels <= 0) {
>>> +        av_log(s, AV_LOG_ERROR, "Invalid number of channels %d\n", channels);
>>> +        return AVERROR_INVALIDDATA;
>>> +    }
>>
>> Since the channel count is read with avio_rb32(), it should have a
>> sanity check for upper limit too. Insanely large channel counts will
>> cause integer overflows.
>
> Right. So what would be a sensible limit here, 100 or so?

AVCodecContext.channel_layout is a 64-bit bitmask...

Patch
diff mbox 

diff --git a/libavformat/au.c b/libavformat/au.c
index 3a83d28..3ae784d 100644
--- a/libavformat/au.c
+++ b/libavformat/au.c
@@ -145,6 +145,11 @@  static int au_read_header(AVFormatContext *s)
         return AVERROR_INVALIDDATA;
     }
 
+    if (channels <= 0) {
+        av_log(s, AV_LOG_ERROR, "Invalid number of channels %d\n", channels);
+        return AVERROR_INVALIDDATA;
+    }
+
     if (size >= 24) {
         /* skip unused data */
         avio_skip(pb, size - 24);