[FFmpeg-devel] lavf/r3d: Check avio_read() return value

Submitted by Carl Eugen Hoyos on Aug. 22, 2019, 10:06 a.m.

Details

Message ID CAB0OVGr6Gy+uG7Q3b67ot=4t6Q8hsF7bcy_CBTkcbjLd82k7mA@mail.gmail.com
State New
Headers show

Commit Message

Carl Eugen Hoyos Aug. 22, 2019, 10:06 a.m.
Hi!

Attached patch fixes usage of uninitialized data reading broken r3d files.

Please review, Carl Eugen

Comments

Reimar Döffinger Aug. 22, 2019, 7:57 p.m.
On Thu, Aug 22, 2019 at 12:06:03PM +0200, Carl Eugen Hoyos wrote:
> Hi!
>
> Attached patch fixes usage of uninitialized data reading broken r3d files.
>
> Please review, Carl Eugen

> From efce9940b890b9cf5a9e7400b05be10f6906fbb1 Mon Sep 17 00:00:00 2001
> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
> Date: Thu, 22 Aug 2019 12:02:36 +0200
> Subject: [PATCH] lavf/r3d: Check avio_read() return value.
>
> Fixes use of uninitialized data reading broken files.
> Reported-by: Anatoly Trosinenko <anatoly dot trosinenko at gmail>
> ---
>  libavformat/r3d.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/r3d.c b/libavformat/r3d.c
> index 224bcf780d..919c3c4960 100644
> --- a/libavformat/r3d.c
> +++ b/libavformat/r3d.c
> @@ -98,7 +98,8 @@ static int r3d_read_red1(AVFormatContext *s)
>      r3d->audio_channels = avio_r8(s->pb); // audio channels
>      av_log(s, AV_LOG_TRACE, "audio channels %d\n", tmp);
>
> -    avio_read(s->pb, filename, 257);
> +    if (avio_read(s->pb, filename, 257) < 0)
> +        return AVERROR_INVALIDDATA;
>      filename[sizeof(filename)-1] = 0;

As far as I can tell most code would either return the avio_read return
value or AVERROR(EIO).
Also I think short reads are also an issue and would
cause the same issue?
Maybe it would be sensible to also change the 257 to
sizeof(filename)-1 while at it?
Overall something along the lines (with possibly less stupid names) of

namebytes = sizeof(filename)-1;
ret = avio_read(s->pb, filename, namebytes);
if (ret < namebytes)
    return ret < 0 ? ret : AVERROR(EIO);
filename[namebytes] = 0;

The hard failure return here seems appropriate as this logic reads the
very start of the file, so if a read error happens here I
don't think there's any way to recover anything.

Patch hide | download patch | download mbox

From efce9940b890b9cf5a9e7400b05be10f6906fbb1 Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
Date: Thu, 22 Aug 2019 12:02:36 +0200
Subject: [PATCH] lavf/r3d: Check avio_read() return value.

Fixes use of uninitialized data reading broken files.
Reported-by: Anatoly Trosinenko <anatoly dot trosinenko at gmail>
---
 libavformat/r3d.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libavformat/r3d.c b/libavformat/r3d.c
index 224bcf780d..919c3c4960 100644
--- a/libavformat/r3d.c
+++ b/libavformat/r3d.c
@@ -98,7 +98,8 @@  static int r3d_read_red1(AVFormatContext *s)
     r3d->audio_channels = avio_r8(s->pb); // audio channels
     av_log(s, AV_LOG_TRACE, "audio channels %d\n", tmp);
 
-    avio_read(s->pb, filename, 257);
+    if (avio_read(s->pb, filename, 257) < 0)
+        return AVERROR_INVALIDDATA;
     filename[sizeof(filename)-1] = 0;
     av_dict_set(&st->metadata, "filename", filename, 0);
 
-- 
2.22.0