wtv: permit root table and first sectors to be located beyond 2GB boundary

Message ID 1374589105-74458-1-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö July 23, 2013, 2:18 p.m.
From: Peter Ross <pross@xvid.org>

---
 libavformat/wtv.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Diego Biurrun July 23, 2013, 3:12 p.m. | #1
On Tue, Jul 23, 2013 at 05:18:25PM +0300, Martin Storsjö wrote:
> --- a/libavformat/wtv.c
> +++ b/libavformat/wtv.c
> @@ -155,7 +155,7 @@ static AVIOContext * wtvfile_open_sector(int first_sector, uint64_t length, int
>  
> -    if (avio_seek(s->pb, first_sector << WTV_SECTOR_BITS, SEEK_SET) < 0)
> +    if (avio_seek(s->pb, (int64_t)first_sector << WTV_SECTOR_BITS, SEEK_SET) < 0)
>          return NULL;
>  
> @@ -958,7 +958,7 @@ static int read_header(AVFormatContext *s)
>  
> -    avio_seek(s->pb, root_sector << WTV_SECTOR_BITS, SEEK_SET);
> +    avio_seek(s->pb, (int64_t)root_sector << WTV_SECTOR_BITS, SEEK_SET);
>      root_size = avio_read(s->pb, root, root_size);

Why not change the type to int64_t instead?

Diego
Rémi Denis-Courmont July 23, 2013, 3:26 p.m. | #2
Le mardi 23 juillet 2013 17:18:25 Martin Storsjö a écrit :
> From: Peter Ross <pross@xvid.org>
> 
> ---
>  libavformat/wtv.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/wtv.c b/libavformat/wtv.c
> index 8b92746..494af3b 100644
> --- a/libavformat/wtv.c
> +++ b/libavformat/wtv.c
> @@ -155,7 +155,7 @@ static AVIOContext * wtvfile_open_sector(int
> first_sector, uint64_t length, int WtvFile *wf;
>      uint8_t *buffer;
> 
> -    if (avio_seek(s->pb, first_sector << WTV_SECTOR_BITS, SEEK_SET) < 0)
> +    if (avio_seek(s->pb, (int64_t)first_sector << WTV_SECTOR_BITS,
> SEEK_SET) < 0) return NULL;

INT64_C(WTV_SECTOR_BITS) ?
Martin Storsjö July 23, 2013, 3:52 p.m. | #3
On Tue, 23 Jul 2013, Diego Biurrun wrote:

> On Tue, Jul 23, 2013 at 05:18:25PM +0300, Martin Storsjö wrote:
>> --- a/libavformat/wtv.c
>> +++ b/libavformat/wtv.c
>> @@ -155,7 +155,7 @@ static AVIOContext * wtvfile_open_sector(int first_sector, uint64_t length, int
>>
>> -    if (avio_seek(s->pb, first_sector << WTV_SECTOR_BITS, SEEK_SET) < 0)
>> +    if (avio_seek(s->pb, (int64_t)first_sector << WTV_SECTOR_BITS, SEEK_SET) < 0)
>>          return NULL;
>>
>> @@ -958,7 +958,7 @@ static int read_header(AVFormatContext *s)
>>
>> -    avio_seek(s->pb, root_sector << WTV_SECTOR_BITS, SEEK_SET);
>> +    avio_seek(s->pb, (int64_t)root_sector << WTV_SECTOR_BITS, SEEK_SET);
>>      root_size = avio_read(s->pb, root, root_size);
>
> Why not change the type to int64_t instead?

Because the variable itself will only ever have a 32 bit value, it's only 
when it's shifted that it needs to be a 64 bit variable.

// Martin
Martin Storsjö July 23, 2013, 3:54 p.m. | #4
On Tue, 23 Jul 2013, Rémi Denis-Courmont wrote:

> Le mardi 23 juillet 2013 17:18:25 Martin Storsjö a écrit :
>> From: Peter Ross <pross@xvid.org>
>> 
>> ---
>>  libavformat/wtv.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/libavformat/wtv.c b/libavformat/wtv.c
>> index 8b92746..494af3b 100644
>> --- a/libavformat/wtv.c
>> +++ b/libavformat/wtv.c
>> @@ -155,7 +155,7 @@ static AVIOContext * wtvfile_open_sector(int
>> first_sector, uint64_t length, int WtvFile *wf;
>>      uint8_t *buffer;
>> 
>> -    if (avio_seek(s->pb, first_sector << WTV_SECTOR_BITS, SEEK_SET) < 0)
>> +    if (avio_seek(s->pb, (int64_t)first_sector << WTV_SECTOR_BITS,
>> SEEK_SET) < 0) return NULL;
>
> INT64_C(WTV_SECTOR_BITS) ?

That would also work. There are a few existing cases in the file already 
that all are avio_seek(.., (int64_t)sector << WTV_SECTOR_BITS), so this 
patch is consistent with that, but if others also prefer your suggestion 
we sure could go with that instead.

// Martin
Diego Biurrun July 23, 2013, 9:21 p.m. | #5
On Tue, Jul 23, 2013 at 06:26:36PM +0300, Rémi Denis-Courmont wrote:
> Le mardi 23 juillet 2013 17:18:25 Martin Storsjö a écrit :
> > --- a/libavformat/wtv.c
> > +++ b/libavformat/wtv.c
> > @@ -155,7 +155,7 @@ static AVIOContext * wtvfile_open_sector(int
> > first_sector, uint64_t length, int WtvFile *wf;
> >      uint8_t *buffer;
> > 
> > -    if (avio_seek(s->pb, first_sector << WTV_SECTOR_BITS, SEEK_SET) < 0)
> > +    if (avio_seek(s->pb, (int64_t)first_sector << WTV_SECTOR_BITS,
> > SEEK_SET) < 0) return NULL;
> 
> INT64_C(WTV_SECTOR_BITS) ?

Sounds cleaner to me than a cast.

Diego
Martin Storsjö July 24, 2013, 9:01 a.m. | #6
On Tue, 23 Jul 2013, Diego Biurrun wrote:

> On Tue, Jul 23, 2013 at 06:26:36PM +0300, Rémi Denis-Courmont wrote:
>> Le mardi 23 juillet 2013 17:18:25 Martin Storsjö a écrit :
>>> --- a/libavformat/wtv.c
>>> +++ b/libavformat/wtv.c
>>> @@ -155,7 +155,7 @@ static AVIOContext * wtvfile_open_sector(int
>>> first_sector, uint64_t length, int WtvFile *wf;
>>>      uint8_t *buffer;
>>>
>>> -    if (avio_seek(s->pb, first_sector << WTV_SECTOR_BITS, SEEK_SET) < 0)
>>> +    if (avio_seek(s->pb, (int64_t)first_sector << WTV_SECTOR_BITS,
>>> SEEK_SET) < 0) return NULL;
>>
>> INT64_C(WTV_SECTOR_BITS) ?
>
> Sounds cleaner to me than a cast.

Actually, it turned out this doesn't work, the define is expanded too 
late:

libavformat/wtv.c:961:45: error: use of undeclared identifier 
'WTV_SECTOR_BITSLL'

Alternatively, one could include the INT64_C in the definition of 
WTV_SECTOR_BITS, but I'm not sure all uses of the define would appreciate 
it being a 64 bit integer. It doesn't cause warnings with clang at least 
though.

// Martin
Diego Biurrun July 26, 2013, 6:04 p.m. | #7
On Wed, Jul 24, 2013 at 12:01:48PM +0300, Martin Storsjö wrote:
> On Tue, 23 Jul 2013, Diego Biurrun wrote:
> >On Tue, Jul 23, 2013 at 06:26:36PM +0300, Rémi Denis-Courmont wrote:
> >>Le mardi 23 juillet 2013 17:18:25 Martin Storsjö a écrit :
> >>>--- a/libavformat/wtv.c
> >>>+++ b/libavformat/wtv.c
> >>>@@ -155,7 +155,7 @@ static AVIOContext * wtvfile_open_sector(int
> >>>first_sector, uint64_t length, int WtvFile *wf;
> >>>     uint8_t *buffer;
> >>>
> >>>-    if (avio_seek(s->pb, first_sector << WTV_SECTOR_BITS, SEEK_SET) < 0)
> >>>+    if (avio_seek(s->pb, (int64_t)first_sector << WTV_SECTOR_BITS,
> >>>SEEK_SET) < 0) return NULL;
> >>
> >>INT64_C(WTV_SECTOR_BITS) ?
> >
> >Sounds cleaner to me than a cast.
> 
> Actually, it turned out this doesn't work, the define is expanded
> too late:
> 
> libavformat/wtv.c:961:45: error: use of undeclared identifier
> 'WTV_SECTOR_BITSLL'
> 
> Alternatively, one could include the INT64_C in the definition of
> WTV_SECTOR_BITS, but I'm not sure all uses of the define would
> appreciate it being a 64 bit integer. It doesn't cause warnings with
> clang at least though.

Do whatever you deem most appropriate.

Diego
Luca Barbato July 26, 2013, 6:12 p.m. | #8
On 26/07/13 20:04, Diego Biurrun wrote:
> On Wed, Jul 24, 2013 at 12:01:48PM +0300, Martin Storsjö wrote:
>> On Tue, 23 Jul 2013, Diego Biurrun wrote:
>>> On Tue, Jul 23, 2013 at 06:26:36PM +0300, Rémi Denis-Courmont wrote:
>>>> Le mardi 23 juillet 2013 17:18:25 Martin Storsjö a écrit :
>>>>> --- a/libavformat/wtv.c
>>>>> +++ b/libavformat/wtv.c
>>>>> @@ -155,7 +155,7 @@ static AVIOContext * wtvfile_open_sector(int
>>>>> first_sector, uint64_t length, int WtvFile *wf;
>>>>>     uint8_t *buffer;
>>>>>
>>>>> -    if (avio_seek(s->pb, first_sector << WTV_SECTOR_BITS, SEEK_SET) < 0)
>>>>> +    if (avio_seek(s->pb, (int64_t)first_sector << WTV_SECTOR_BITS,
>>>>> SEEK_SET) < 0) return NULL;
>>>>
>>>> INT64_C(WTV_SECTOR_BITS) ?
>>>
>>> Sounds cleaner to me than a cast.
>>
>> Actually, it turned out this doesn't work, the define is expanded
>> too late:
>>
>> libavformat/wtv.c:961:45: error: use of undeclared identifier
>> 'WTV_SECTOR_BITSLL'
>>
>> Alternatively, one could include the INT64_C in the definition of
>> WTV_SECTOR_BITS, but I'm not sure all uses of the define would
>> appreciate it being a 64 bit integer. It doesn't cause warnings with
>> clang at least though.
> 
> Do whatever you deem most appropriate.

libavformat/wtv.c:                (wf->sectors[i] != wf->sectors[i - 1]
+ (1 << (wf->sector_bits - WTV_SECTOR_BITS)) &&
libavformat/wtv.c:                avio_seek(pb, (int64_t)wf->sectors[i]
<< WTV_SECTOR_BITS, SEEK_SET) < 0)) {
libavformat/wtv.c:                avio_seek(pb,
((int64_t)wf->sectors[offset >> wf->sector_bits] << WTV_SECTOR_BITS)
libavformat/wtv.c:    if (avio_seek(s->pb, first_sector <<
WTV_SECTOR_BITS, SEEK_SET) < 0)
libavformat/wtv.c:        wf->sector_bits = WTV_SECTOR_BITS;
libavformat/wtv.c:        wf->sector_bits = length & (1ULL<<63) ?
WTV_SECTOR_BITS : WTV_BIGSECTOR_BITS;
libavformat/wtv.c:        wf->sectors = av_malloc(nb_sectors1 <<
WTV_SECTOR_BITS);
libavformat/wtv.c:            if (avio_seek(s->pb, (int64_t)sectors1[i]
<< WTV_SECTOR_BITS, SEEK_SET) < 0)
libavformat/wtv.c:        wf->sector_bits = length & (1ULL<<63) ?
WTV_SECTOR_BITS : WTV_BIGSECTOR_BITS;
libavformat/wtv.c:    if (avio_seek(s->pb, (int64_t)wf->sectors[0] <<
WTV_SECTOR_BITS, SEEK_SET) < 0) {
libavformat/wtv.c:    avio_seek(s->pb, root_sector << WTV_SECTOR_BITS,
SEEK_SET

Given the usage feels better making that constant 64bit and drop some casts.

lu

Patch

diff --git a/libavformat/wtv.c b/libavformat/wtv.c
index 8b92746..494af3b 100644
--- a/libavformat/wtv.c
+++ b/libavformat/wtv.c
@@ -155,7 +155,7 @@  static AVIOContext * wtvfile_open_sector(int first_sector, uint64_t length, int
     WtvFile *wf;
     uint8_t *buffer;
 
-    if (avio_seek(s->pb, first_sector << WTV_SECTOR_BITS, SEEK_SET) < 0)
+    if (avio_seek(s->pb, (int64_t)first_sector << WTV_SECTOR_BITS, SEEK_SET) < 0)
         return NULL;
 
     wf = av_mallocz(sizeof(WtvFile));
@@ -958,7 +958,7 @@  static int read_header(AVFormatContext *s)
     avio_skip(s->pb, 4);
     root_sector = avio_rl32(s->pb);
 
-    avio_seek(s->pb, root_sector << WTV_SECTOR_BITS, SEEK_SET);
+    avio_seek(s->pb, (int64_t)root_sector << WTV_SECTOR_BITS, SEEK_SET);
     root_size = avio_read(s->pb, root, root_size);
     if (root_size < 0)
         return AVERROR_INVALIDDATA;