| Message ID | 20181213025914.206807-1-chcunningham@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
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
> > 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.
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; }