[FFmpeg-devel] Fix signed integer overflows

Submitted by Vitaly Buka on Aug. 20, 2017, 6:56 p.m.

Details

Message ID 20170820185647.42927-1-vitalybuka@google.com
State New
Headers show

Commit Message

Vitaly Buka Aug. 20, 2017, 6:56 p.m.
Signed integer overflow is undefined behavior.
Detected with clang and -fsanitize=signed-integer-overflow

Signed-off-by: Vitaly Buka <vitalybuka@google.com>
---
 libavcodec/utils.c    | 2 +-
 libavformat/aviobuf.c | 2 ++
 libavformat/mov.c     | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

Comments

Vitaly Buka Aug. 22, 2017, 5:13 p.m.
What else can I do yo make it accepted?

On Sun, Aug 20, 2017 at 11:56 AM, Vitaly Buka <vitalybuka@google.com> wrote:

> Signed integer overflow is undefined behavior.
> Detected with clang and -fsanitize=signed-integer-overflow
>
> Signed-off-by: Vitaly Buka <vitalybuka@google.com>
> ---
>  libavcodec/utils.c    | 2 +-
>  libavformat/aviobuf.c | 2 ++
>  libavformat/mov.c     | 2 +-
>  3 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 1336e921c9..1b8ad1d200 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -971,7 +971,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>          }
>
>          if (!avctx->rc_initial_buffer_occupancy)
> -            avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size *
> 3 / 4;
> +            avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size *
> 3LL / 4;
>
>          if (avctx->ticks_per_frame && avctx->time_base.num &&
>              avctx->ticks_per_frame > INT_MAX / avctx->time_base.num) {
> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> index 7f4e740a33..ec21fc7d38 100644
> --- a/libavformat/aviobuf.c
> +++ b/libavformat/aviobuf.c
> @@ -259,6 +259,8 @@ int64_t avio_seek(AVIOContext *s, int64_t offset, int
> whence)
>          offset1 = pos + (s->buf_ptr - s->buffer);
>          if (offset == 0)
>              return offset1;
> +        if (offset > INT64_MAX - offset1)
> +            return AVERROR(EINVAL);
>          offset += offset1;
>      }
>      if (offset < 0)
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 522ce60c2d..a14c9f182b 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -5572,7 +5572,7 @@ static int mov_read_default(MOVContext *c,
> AVIOContext *pb, MOVAtom atom)
>
>      if (atom.size < 0)
>          atom.size = INT64_MAX;
> -    while (total_size + 8 <= atom.size && !avio_feof(pb)) {
> +    while (total_size <= atom.size - 8 && !avio_feof(pb)) {
>          int (*parse)(MOVContext*, AVIOContext*, MOVAtom) = NULL;
>          a.size = atom.size;
>          a.type=0;
> --
> 2.14.1.480.gb18f417b89-goog
>
>
Paul B Mahol Aug. 22, 2017, 5:30 p.m.
On 8/22/17, Vitaly Buka <vitalybuka-at-google.com@ffmpeg.org> wrote:
> What else can I do yo make it accepted?

Ping it after month or two?

>
> On Sun, Aug 20, 2017 at 11:56 AM, Vitaly Buka <vitalybuka@google.com> wrote:
>
>> Signed integer overflow is undefined behavior.
>> Detected with clang and -fsanitize=signed-integer-overflow
>>
>> Signed-off-by: Vitaly Buka <vitalybuka@google.com>
>> ---
>>  libavcodec/utils.c    | 2 +-
>>  libavformat/aviobuf.c | 2 ++
>>  libavformat/mov.c     | 2 +-
>>  3 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> index 1336e921c9..1b8ad1d200 100644
>> --- a/libavcodec/utils.c
>> +++ b/libavcodec/utils.c
>> @@ -971,7 +971,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>          }
>>
>>          if (!avctx->rc_initial_buffer_occupancy)
>> -            avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size *
>> 3 / 4;
>> +            avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size *
>> 3LL / 4;
>>
>>          if (avctx->ticks_per_frame && avctx->time_base.num &&
>>              avctx->ticks_per_frame > INT_MAX / avctx->time_base.num) {
>> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
>> index 7f4e740a33..ec21fc7d38 100644
>> --- a/libavformat/aviobuf.c
>> +++ b/libavformat/aviobuf.c
>> @@ -259,6 +259,8 @@ int64_t avio_seek(AVIOContext *s, int64_t offset, int
>> whence)
>>          offset1 = pos + (s->buf_ptr - s->buffer);
>>          if (offset == 0)
>>              return offset1;
>> +        if (offset > INT64_MAX - offset1)
>> +            return AVERROR(EINVAL);
>>          offset += offset1;
>>      }
>>      if (offset < 0)
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 522ce60c2d..a14c9f182b 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -5572,7 +5572,7 @@ static int mov_read_default(MOVContext *c,
>> AVIOContext *pb, MOVAtom atom)
>>
>>      if (atom.size < 0)
>>          atom.size = INT64_MAX;
>> -    while (total_size + 8 <= atom.size && !avio_feof(pb)) {
>> +    while (total_size <= atom.size - 8 && !avio_feof(pb)) {
>>          int (*parse)(MOVContext*, AVIOContext*, MOVAtom) = NULL;
>>          a.size = atom.size;
>>          a.type=0;
>> --
>> 2.14.1.480.gb18f417b89-goog
>>
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
James Almer Aug. 22, 2017, 5:31 p.m.
On 8/22/2017 2:30 PM, Paul B Mahol wrote:
> On 8/22/17, Vitaly Buka <vitalybuka-at-google.com@ffmpeg.org> wrote:
>> What else can I do yo make it accepted?
> 
> Ping it after month or two?

A week is enough wait to justify a ping. A month is overkill.

> 
>>
>> On Sun, Aug 20, 2017 at 11:56 AM, Vitaly Buka <vitalybuka@google.com> wrote:
>>
>>> Signed integer overflow is undefined behavior.
>>> Detected with clang and -fsanitize=signed-integer-overflow
>>>
>>> Signed-off-by: Vitaly Buka <vitalybuka@google.com>
>>> ---
>>>  libavcodec/utils.c    | 2 +-
>>>  libavformat/aviobuf.c | 2 ++
>>>  libavformat/mov.c     | 2 +-
>>>  3 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>>> index 1336e921c9..1b8ad1d200 100644
>>> --- a/libavcodec/utils.c
>>> +++ b/libavcodec/utils.c
>>> @@ -971,7 +971,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>          }
>>>
>>>          if (!avctx->rc_initial_buffer_occupancy)
>>> -            avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size *
>>> 3 / 4;
>>> +            avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size *
>>> 3LL / 4;
>>>
>>>          if (avctx->ticks_per_frame && avctx->time_base.num &&
>>>              avctx->ticks_per_frame > INT_MAX / avctx->time_base.num) {
>>> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
>>> index 7f4e740a33..ec21fc7d38 100644
>>> --- a/libavformat/aviobuf.c
>>> +++ b/libavformat/aviobuf.c
>>> @@ -259,6 +259,8 @@ int64_t avio_seek(AVIOContext *s, int64_t offset, int
>>> whence)
>>>          offset1 = pos + (s->buf_ptr - s->buffer);
>>>          if (offset == 0)
>>>              return offset1;
>>> +        if (offset > INT64_MAX - offset1)
>>> +            return AVERROR(EINVAL);
>>>          offset += offset1;
>>>      }
>>>      if (offset < 0)
>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>> index 522ce60c2d..a14c9f182b 100644
>>> --- a/libavformat/mov.c
>>> +++ b/libavformat/mov.c
>>> @@ -5572,7 +5572,7 @@ static int mov_read_default(MOVContext *c,
>>> AVIOContext *pb, MOVAtom atom)
>>>
>>>      if (atom.size < 0)
>>>          atom.size = INT64_MAX;
>>> -    while (total_size + 8 <= atom.size && !avio_feof(pb)) {
>>> +    while (total_size <= atom.size - 8 && !avio_feof(pb)) {
>>>          int (*parse)(MOVContext*, AVIOContext*, MOVAtom) = NULL;
>>>          a.size = atom.size;
>>>          a.type=0;
>>> --
>>> 2.14.1.480.gb18f417b89-goog
>>>
>>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer Aug. 23, 2017, 1:15 a.m.
On Sun, Aug 20, 2017 at 11:56:47AM -0700, Vitaly Buka wrote:
> Signed integer overflow is undefined behavior.
> Detected with clang and -fsanitize=signed-integer-overflow
> 
> Signed-off-by: Vitaly Buka <vitalybuka@google.com>
> ---
>  libavcodec/utils.c    | 2 +-
>  libavformat/aviobuf.c | 2 ++
>  libavformat/mov.c     | 2 +-
>  3 files changed, 4 insertions(+), 2 deletions(-)

split and applied

thx

[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index 1336e921c9..1b8ad1d200 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -971,7 +971,7 @@  FF_ENABLE_DEPRECATION_WARNINGS
         }
 
         if (!avctx->rc_initial_buffer_occupancy)
-            avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size * 3 / 4;
+            avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size * 3LL / 4;
 
         if (avctx->ticks_per_frame && avctx->time_base.num &&
             avctx->ticks_per_frame > INT_MAX / avctx->time_base.num) {
diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index 7f4e740a33..ec21fc7d38 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -259,6 +259,8 @@  int64_t avio_seek(AVIOContext *s, int64_t offset, int whence)
         offset1 = pos + (s->buf_ptr - s->buffer);
         if (offset == 0)
             return offset1;
+        if (offset > INT64_MAX - offset1)
+            return AVERROR(EINVAL);
         offset += offset1;
     }
     if (offset < 0)
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 522ce60c2d..a14c9f182b 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -5572,7 +5572,7 @@  static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 
     if (atom.size < 0)
         atom.size = INT64_MAX;
-    while (total_size + 8 <= atom.size && !avio_feof(pb)) {
+    while (total_size <= atom.size - 8 && !avio_feof(pb)) {
         int (*parse)(MOVContext*, AVIOContext*, MOVAtom) = NULL;
         a.size = atom.size;
         a.type=0;