[3/8] mov: Fix parsing short loci

Message ID 1463569341-73691-3-git-send-email-martin@martin.st
State Committed
Headers show

Commit Message

Martin Storsjö May 18, 2016, 11:02 a.m.
From: Michael Niedermayer <michaelni@gmx.at>

Previously, we required the minimum number of bytes required for
the full box. Don't strictly require the astronomical body and additional
notes fields, but do require an altitude field (which currently isn't
parsed). This matches the initial length check at the start of the function
(which doesn't know about the variable length place field).
---
 libavformat/mov.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Vittorio Giovara May 18, 2016, 4:07 p.m. | #1
On Wed, May 18, 2016 at 7:02 AM, Martin Storsjö <martin@martin.st> wrote:
> From: Michael Niedermayer <michaelni@gmx.at>
>
> Previously, we required the minimum number of bytes required for
> the full box. Don't strictly require the astronomical body and additional
> notes fields, but do require an altitude field (which currently isn't
> parsed). This matches the initial length check at the start of the function
> (which doesn't know about the variable length place field).
> ---
>  libavformat/mov.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 42a5220..81803cf 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -244,7 +244,7 @@ static int mov_metadata_loci(MOVContext *c, AVIOContext *pb, unsigned len)
>      avio_skip(pb, 1); // role
>      len -= 1;
>
> -    if (len < 14) {
> +    if (len < 12) {
>          av_log(c->fc, AV_LOG_ERROR, "no space for coordinates left (%d)\n", len);
>          return AVERROR_INVALIDDATA;
>      }
> --
> 2.7.4 (Apple Git-66)

Should this value be factored out with a define (LOCI_SIZE) while at it?
Probably ok either way
Martin Storsjö May 18, 2016, 7:18 p.m. | #2
On Wed, 18 May 2016, Vittorio Giovara wrote:

> On Wed, May 18, 2016 at 7:02 AM, Martin Storsjö <martin@martin.st> wrote:
>> From: Michael Niedermayer <michaelni@gmx.at>
>>
>> Previously, we required the minimum number of bytes required for
>> the full box. Don't strictly require the astronomical body and additional
>> notes fields, but do require an altitude field (which currently isn't
>> parsed). This matches the initial length check at the start of the function
>> (which doesn't know about the variable length place field).
>> ---
>>  libavformat/mov.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 42a5220..81803cf 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -244,7 +244,7 @@ static int mov_metadata_loci(MOVContext *c, AVIOContext *pb, unsigned len)
>>      avio_skip(pb, 1); // role
>>      len -= 1;
>>
>> -    if (len < 14) {
>> +    if (len < 12) {
>>          av_log(c->fc, AV_LOG_ERROR, "no space for coordinates left (%d)\n", len);
>>          return AVERROR_INVALIDDATA;
>>      }
>> --
>> 2.7.4 (Apple Git-66)
>
> Should this value be factored out with a define (LOCI_SIZE) while at it?

No, not really.

This box has got a few fixed-length fields, and a few variable length 
ones.

At the top of the function, we check that the box is at least the minimum 
size if all the variable length fields are at their smallest, and skipping 
the last few fields that can be omitted. This helps sorting out those 
boxes that are guaranteed to not be large enough, and avoids checking the 
length for every step at the start of the function.

Now after this, after reading a variable length field, we want to check 
whether the remainder is large enough for the rest of the box. Previously 
this was 14 bytes, 3 * 32bit fields, plus two null terminated strings 
which take at least 2 bytes each, for each null termination, i.e. 14 
bytes. Now apparently, somebody somewhere has written boxes that lack 
these last two strings. So we should only check for the 3 * 32 bit fields 
instead.

So what we really check here is "do we at least have enough bytes for 3 
times avio_rb32() below?". That's not LOCI_SIZE.

// Martin
Luca Barbato May 18, 2016, 7:55 p.m. | #3
On 18/05/16 21:18, Martin Storsjö wrote:
> So what we really check here is "do we at least have enough bytes for 3
> times avio_rb32() below?". That's not LOCI_SIZE.

That would warrant a short comment maybe.

lu
Martin Storsjö May 18, 2016, 7:58 p.m. | #4
On Wed, 18 May 2016, Luca Barbato wrote:

> On 18/05/16 21:18, Martin Storsjö wrote:
>> So what we really check here is "do we at least have enough bytes for 3
>> times avio_rb32() below?". That's not LOCI_SIZE.
>
> That would warrant a short comment maybe.

No, I don't think so.

The pattern is common, it's like this:

     if (len < 4)
         return err;
     avio_rb32(pb);
     len -= 4;
     if (len < 12)
         return err;
     avio_rb32(pb);
     avio_rb32(pb);
     avio_rb32(pb);
     len -= 12;

I'm not wasting space on a comment for how that works.

// Martin
Martin Storsjö May 18, 2016, 8:15 p.m. | #5
On Wed, 18 May 2016, Martin Storsjö wrote:

> On Wed, 18 May 2016, Luca Barbato wrote:
>
>> On 18/05/16 21:18, Martin Storsjö wrote:
>>> So what we really check here is "do we at least have enough bytes for 3
>>> times avio_rb32() below?". That's not LOCI_SIZE.
>>
>> That would warrant a short comment maybe.
>
> No, I don't think so.
>
> The pattern is common, it's like this:
>
>     if (len < 4)
>         return err;
>     avio_rb32(pb);
>     len -= 4;
>     if (len < 12)
>         return err;
>     avio_rb32(pb);
>     avio_rb32(pb);
>     avio_rb32(pb);
>     len -= 12;
>
> I'm not wasting space on a comment for how that works.

To clarify; at this particular commit it might not be totally clear, but 
it gets clearer after 5/8. What might use a comment is the minimum box 
size check at the start of the function, but that's unrelated to this 
patch, although I don't see a pressing need for that either.

// Martin
Luca Barbato May 18, 2016, 10:23 p.m. | #6
On 18/05/16 22:15, Martin Storsjö wrote:
> To clarify; at this particular commit it might not be totally clear, but
> it gets clearer after 5/8. What might use a comment is the minimum box
> size check at the start of the function, but that's unrelated to this
> patch, although I don't see a pressing need for that either.

Ok.
Vittorio Giovara May 18, 2016, 11:24 p.m. | #7
On Wed, May 18, 2016 at 3:18 PM, Martin Storsjö <martin@martin.st> wrote:
> On Wed, 18 May 2016, Vittorio Giovara wrote:
>
>> On Wed, May 18, 2016 at 7:02 AM, Martin Storsjö <martin@martin.st> wrote:
>>>
>>> From: Michael Niedermayer <michaelni@gmx.at>
>>>
>>> Previously, we required the minimum number of bytes required for
>>> the full box. Don't strictly require the astronomical body and additional
>>> notes fields, but do require an altitude field (which currently isn't
>>> parsed). This matches the initial length check at the start of the
>>> function
>>> (which doesn't know about the variable length place field).
>>> ---
>>>  libavformat/mov.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>> index 42a5220..81803cf 100644
>>> --- a/libavformat/mov.c
>>> +++ b/libavformat/mov.c
>>> @@ -244,7 +244,7 @@ static int mov_metadata_loci(MOVContext *c,
>>> AVIOContext *pb, unsigned len)
>>>      avio_skip(pb, 1); // role
>>>      len -= 1;
>>>
>>> -    if (len < 14) {
>>> +    if (len < 12) {
>>>          av_log(c->fc, AV_LOG_ERROR, "no space for coordinates left
>>> (%d)\n", len);
>>>          return AVERROR_INVALIDDATA;
>>>      }
>>> --
>>> 2.7.4 (Apple Git-66)
>>
>>
>> Should this value be factored out with a define (LOCI_SIZE) while at it?
>
>
> No, not really.
>
> This box has got a few fixed-length fields, and a few variable length ones.
>
> At the top of the function, we check that the box is at least the minimum
> size if all the variable length fields are at their smallest, and skipping
> the last few fields that can be omitted. This helps sorting out those boxes
> that are guaranteed to not be large enough, and avoids checking the length
> for every step at the start of the function.
>
> Now after this, after reading a variable length field, we want to check
> whether the remainder is large enough for the rest of the box. Previously
> this was 14 bytes, 3 * 32bit fields, plus two null terminated strings which
> take at least 2 bytes each, for each null termination, i.e. 14 bytes. Now
> apparently, somebody somewhere has written boxes that lack these last two
> strings. So we should only check for the 3 * 32 bit fields instead.
>
> So what we really check here is "do we at least have enough bytes for 3
> times avio_rb32() below?". That's not LOCI_SIZE.

I see, thanks for the explanation.

Patch

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 42a5220..81803cf 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -244,7 +244,7 @@  static int mov_metadata_loci(MOVContext *c, AVIOContext *pb, unsigned len)
     avio_skip(pb, 1); // role
     len -= 1;
 
-    if (len < 14) {
+    if (len < 12) {
         av_log(c->fc, AV_LOG_ERROR, "no space for coordinates left (%d)\n", len);
         return AVERROR_INVALIDDATA;
     }