[FFmpeg-devel] [PATCH] avformat/mov: fix undefined behavior in mov_read_default

[Andreas Rheinhardt]

Zhao Zhili:
> 
> 
>> On Jul 4, 2020, at 11:32 PM, Andreas Rheinhardt <andreas.rheinhardt at gmail.com> wrote:
>>
>> Zhao Zhili:
>>> ---
>>> libavformat/mov.c | 11 +++++------
>>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>> index df5bebdff1..da438e4e2c 100644
>>> --- a/libavformat/mov.c
>>> +++ b/libavformat/mov.c
>>> @@ -6945,13 +6945,12 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>>                   a.type == MKTAG('h','o','o','v')) &&
>>>                 a.size >= 8 &&
>>>                 c->fc->strict_std_compliance < FF_COMPLIANCE_STRICT) {
>>> -                uint8_t buf[8];
>>> -                uint32_t *type = (uint32_t *)buf + 1;
>>> -                if (avio_read(pb, buf, 8) != 8)
>>> -                    return AVERROR_INVALIDDATA;
>>> +                uint32_t type;
>>> +                avio_skip(pb, 4);
>>> +                type = avio_rl32(pb);
>>>                 avio_seek(pb, -8, SEEK_CUR);
>>> -                if (*type == MKTAG('m','v','h','d') ||
>>> -                    *type == MKTAG('c','m','o','v')) {
>>> +                if (type == MKTAG('m','v','h','d') ||
>>> +                    type == MKTAG('c','m','o','v')) {
>>>                     av_log(c->fc, AV_LOG_ERROR, "Detected moov in a free or hoov atom.\n");
>>>                     a.type = MKTAG('m','o','o','v');
>>>                 }
>>>
>> 1. The undefined behaviour you are talking about is that buf needn't
>> have the right alignment for an uint32_t, isn't it? This should be
>> mentioned in the commit message instead of the generic "undefined
>> behaviour".
> 
> Will replace the commit message by "fix undefined behavior of casting
> uint8_t* to unsigned*".
> 
Better use something that has "unaligned" in the title like "Fix
unaligned read of uint32_t".

>> 2. Your code won't work on big endian.
> 
> Could you elaborate on that? From what I read, both avio_rl32 and MKTAG
> are making bytes as little endian, it doesn't matter what the native
> endian is.
> 
Sorry, my bad. You're right. Actually if I am not mistaken, the current
code doesn't work on BE (or rather, it works differently on BE than LE)
and your patch fixes that, too. That should be in the commit message, too.

- Andreas