diff mbox series

[FFmpeg-devel] avformat/wtvdec: Fix reading OLE dates on BE

Message ID 20210228030122.3101246-1-andreas.rheinhardt@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/wtvdec: Fix reading OLE dates on BE
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Feb. 28, 2021, 3:01 a.m. UTC
The WTV demuxer's oledata_to_iso8601 reads a value via avio_rl64
and reinterprets it as a double via av_int2double. This does not
work on big endian systems. So swap it to native endianness before
av_int2double.

law-and-order-partial.wtv from the FATE-suite exhibited the problem.

Thanks-to: Andriy Gelman <andriy.gelman@gmail.com>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/wtvdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Peter Ross Feb. 28, 2021, 3:47 a.m. UTC | #1
On Sun, Feb 28, 2021 at 04:01:21AM +0100, Andreas Rheinhardt wrote:
> The WTV demuxer's oledata_to_iso8601 reads a value via avio_rl64
> and reinterprets it as a double via av_int2double. This does not
> work on big endian systems. So swap it to native endianness before
> av_int2double.
> 
> law-and-order-partial.wtv from the FATE-suite exhibited the problem.
> 
> Thanks-to: Andriy Gelman <andriy.gelman@gmail.com>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/wtvdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c
> index 7def9d2348..bb84e5dc9f 100644
> --- a/libavformat/wtvdec.c
> +++ b/libavformat/wtvdec.c
> @@ -418,7 +418,7 @@ static int crazytime_to_iso8601(char *buf, int buf_size, int64_t value)
>   */
>  static int oledate_to_iso8601(char *buf, int buf_size, int64_t value)
>  {
> -    time_t t = (av_int2double(value) - 25569.0) * 86400;
> +    time_t t = (av_int2double(av_le2ne64(value)) - 25569.0) * 86400;
>      struct tm tmbuf;
>      struct tm *tm= gmtime_r(&t, &tmbuf);
>      if (!tm)
> -- 
> 2.27.0

ok, please apply.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
Andriy Gelman Feb. 28, 2021, 2:51 p.m. UTC | #2
On Sun, 28. Feb 04:01, Andreas Rheinhardt wrote:
> The WTV demuxer's oledata_to_iso8601 reads a value via avio_rl64
> and reinterprets it as a double via av_int2double. This does not
> work on big endian systems. So swap it to native endianness before
> av_int2double.
> 
> law-and-order-partial.wtv from the FATE-suite exhibited the problem.
> 
> Thanks-to: Andriy Gelman <andriy.gelman@gmail.com>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/wtvdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c
> index 7def9d2348..bb84e5dc9f 100644
> --- a/libavformat/wtvdec.c
> +++ b/libavformat/wtvdec.c
> @@ -418,7 +418,7 @@ static int crazytime_to_iso8601(char *buf, int buf_size, int64_t value)
>   */
>  static int oledate_to_iso8601(char *buf, int buf_size, int64_t value)
>  {
> -    time_t t = (av_int2double(value) - 25569.0) * 86400;
> +    time_t t = (av_int2double(av_le2ne64(value)) - 25569.0) * 86400;
>      struct tm tmbuf;
>      struct tm *tm= gmtime_r(&t, &tmbuf);
>      if (!tm)

I don't think that's correct because the output of avio_rl64() is already native
endian.

The result of:
double a = (av_int2double(value) - 25569.0) * 86400;
is consistent on my x86_64 and PPC64.

The failing fate test seems to be because sizeof(time_t) = 4 on the BE PPC64
docker instance, but is 8 on my x86_64.
Andreas Rheinhardt Feb. 28, 2021, 2:58 p.m. UTC | #3
Peter Ross:
> On Sun, Feb 28, 2021 at 04:01:21AM +0100, Andreas Rheinhardt wrote:
>> The WTV demuxer's oledata_to_iso8601 reads a value via avio_rl64
>> and reinterprets it as a double via av_int2double. This does not
>> work on big endian systems. So swap it to native endianness before
>> av_int2double.
>>
>> law-and-order-partial.wtv from the FATE-suite exhibited the problem.
>>
>> Thanks-to: Andriy Gelman <andriy.gelman@gmail.com>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavformat/wtvdec.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c
>> index 7def9d2348..bb84e5dc9f 100644
>> --- a/libavformat/wtvdec.c
>> +++ b/libavformat/wtvdec.c
>> @@ -418,7 +418,7 @@ static int crazytime_to_iso8601(char *buf, int buf_size, int64_t value)
>>   */
>>  static int oledate_to_iso8601(char *buf, int buf_size, int64_t value)
>>  {
>> -    time_t t = (av_int2double(value) - 25569.0) * 86400;
>> +    time_t t = (av_int2double(av_le2ne64(value)) - 25569.0) * 86400;
>>      struct tm tmbuf;
>>      struct tm *tm= gmtime_r(&t, &tmbuf);
>>      if (!tm)
>> -- 
>> 2.27.0
> 
> ok, please apply.
> 
Unfortunately, I was wrong. avio_rl64 already implicitly performs the
necessary byteswap. The actual problem is that the date in
law-and-order-partial.wtv (9999-12-31 23:59:59) is outside the range
representable by a 32bit time_t. So we have to write our own gmtime_r to
fix this. Btw: A non-representable float->int conversion is undefined
behaviour.
(The Matroska fate test which started all this could of course also be
fixed by simply deleting the relevant metadata entry, but I don't want
to do this.)

- Andreas

PS: Thanks to Andriy for his continued involvement in the investigation
of this. And for pointing out my error.
diff mbox series

Patch

diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c
index 7def9d2348..bb84e5dc9f 100644
--- a/libavformat/wtvdec.c
+++ b/libavformat/wtvdec.c
@@ -418,7 +418,7 @@  static int crazytime_to_iso8601(char *buf, int buf_size, int64_t value)
  */
 static int oledate_to_iso8601(char *buf, int buf_size, int64_t value)
 {
-    time_t t = (av_int2double(value) - 25569.0) * 86400;
+    time_t t = (av_int2double(av_le2ne64(value)) - 25569.0) * 86400;
     struct tm tmbuf;
     struct tm *tm= gmtime_r(&t, &tmbuf);
     if (!tm)