| Message ID | 20191129194411.20168-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 35005a4af1ebe1ec1b5724121a03036705417fc7 |
| Headers | show |
On Fri, Nov 29, 2019 at 08:44:09PM +0100, Andreas Rheinhardt wrote: > During parsing a flac picture metadata block, the mimetype is read as > follows: Its 32b size field is read and checked for being in the range > 1..63; afterwards, the actual mimetype-string is read into a buffer of > size 64, where the length to read is the minimum of the length field and > the size of the destination buffer -1. Then an assert guards that length > is indeed < the size of the destination buffer before the string in the > buffer is zero-terminated. > > The FFMIN as well as the assert are actually redundant, as it has > been checked that the string (even after terminating) fits into the > buffer. In order to make this clear, reword the check "len >= 64" to > "len >= sizeof(mimetype)" and drop the FFMIN as well as the assert. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/flac_picture.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) will apply patchset thx [...]
diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c index f0871d9c79..6463a370c8 100644 --- a/libavformat/flac_picture.c +++ b/libavformat/flac_picture.c @@ -19,7 +19,6 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ -#include "libavutil/avassert.h" #include "libavutil/intreadwrite.h" #include "libavcodec/png.h" #include "avformat.h" @@ -54,15 +53,14 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size) /* picture mimetype */ len = avio_rb32(pb); - if (len <= 0 || len >= 64 || - avio_read(pb, mimetype, FFMIN(len, sizeof(mimetype) - 1)) != len) { + if (len <= 0 || len >= sizeof(mimetype) || + avio_read(pb, mimetype, len) != len) { av_log(s, AV_LOG_ERROR, "Could not read mimetype from an attached " "picture.\n"); if (s->error_recognition & AV_EF_EXPLODE) ret = AVERROR_INVALIDDATA; goto fail; } - av_assert0(len < sizeof(mimetype)); mimetype[len] = 0; while (mime->id != AV_CODEC_ID_NONE) {
During parsing a flac picture metadata block, the mimetype is read as follows: Its 32b size field is read and checked for being in the range 1..63; afterwards, the actual mimetype-string is read into a buffer of size 64, where the length to read is the minimum of the length field and the size of the destination buffer -1. Then an assert guards that length is indeed < the size of the destination buffer before the string in the buffer is zero-terminated. The FFMIN as well as the assert are actually redundant, as it has been checked that the string (even after terminating) fits into the buffer. In order to make this clear, reword the check "len >= 64" to "len >= sizeof(mimetype)" and drop the FFMIN as well as the assert. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/flac_picture.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)