[FFmpeg-devel] lavf/id3v2: fail read_apic on EOF reading mimetype

Submitted by chcunningham@chromium.org on Dec. 13, 2018, 2:59 a.m.

Details

Message ID 20181213025914.206807-1-chcunningham@chromium.org
State Accepted
Headers show

Commit Message

chcunningham@chromium.org Dec. 13, 2018, 2:59 a.m.
avio_read may return EOF, leaving the mimetype array unitialized. fail
early when this occurs to avoid using the array in an unitialized state.
---
 libavformat/id3v2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Tomas Härdin Dec. 13, 2018, 2:12 p.m.
ons 2018-12-12 klockan 18:59 -0800 skrev chcunningham:
> avio_read may return EOF, leaving the mimetype array unitialized. fail
> early when this occurs to avoid using the array in an unitialized state.
> ---
>  libavformat/id3v2.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> index f7de26a1d8..7c4d1f8677 100644
> --- a/libavformat/id3v2.c
> +++ b/libavformat/id3v2.c
> @@ -612,7 +612,9 @@ static void read_apic(AVFormatContext *s, AVIOContext *pb, int taglen,
>      if (isv34) {
>          taglen -= avio_get_str(pb, taglen, mimetype, sizeof(mimetype));
>      } else {
> -        avio_read(pb, mimetype, 3);
> +        if (avio_read(pb, mimetype, 3) < 0)
> +            goto fail;
> +
>          mimetype[3] = 0;
>          taglen    -= 3;
>      }

Yet another problem that could have been caught by static analysis..
Wouldn't it be better to always leave the array in a valid state?

/Tomas
chcunningham@chromium.org Dec. 13, 2018, 7:18 p.m.
>
> Yet another problem that could have been caught by static analysis..
> Wouldn't it be better to always leave the array in a valid state?
>

Will add that in the next patch. It has the extra benefit of protecting the
isv34 branch.

Goto fail; skips a lot of lines that aren't needed if mimetype is empty, so
I think its worth keeping as well. I'd love to do similar for the isv34
branch, but I'm not sure how to detect the condition given the EOF behavior
for  avio_get_str:

 * @return number of bytes read (is always <= maxlen).
 * If reading ends on EOF or error, the return value will be one more than
 * bytes actually read.

How do callers differentiate between cases where you read 5 bytes vs
reading just 4 bytes and hitting an error - IIIUC both cases return 5.

Patch hide | download patch | download mbox

diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
index f7de26a1d8..7c4d1f8677 100644
--- a/libavformat/id3v2.c
+++ b/libavformat/id3v2.c
@@ -612,7 +612,9 @@  static void read_apic(AVFormatContext *s, AVIOContext *pb, int taglen,
     if (isv34) {
         taglen -= avio_get_str(pb, taglen, mimetype, sizeof(mimetype));
     } else {
-        avio_read(pb, mimetype, 3);
+        if (avio_read(pb, mimetype, 3) < 0)
+            goto fail;
+
         mimetype[3] = 0;
         taglen    -= 3;
     }