| Message ID | 1588173257-14531-2-git-send-email-lance.lmwang@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/5] avformat/dashenc: fix invalid pointer access if avio_get_dyn_buf failed | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
lance.lmwang@gmail.com (12020-04-29): > From: Limin Wang <lance.lmwang@gmail.com> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > --- > libavformat/matroskaenc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c > index 784973a951..f0474da44f 100644 > --- a/libavformat/matroskaenc.c > +++ b/libavformat/matroskaenc.c > @@ -374,9 +374,12 @@ static void end_ebml_master_crc32(AVIOContext *pb, AVIOContext **dyn_cp, > put_ebml_length(pb, size, length_size); > if (mkv->write_crc) { > skip = 6; /* Skip reserved 6-byte long void element from the dynamic buffer. */ > + if (size > skip) { > AV_WL32(crc, av_crc(av_crc_get_table(AV_CRC_32_IEEE_LE), UINT32_MAX, buf + skip, size - skip) ^ UINT32_MAX); > put_ebml_binary(pb, EBML_ID_CRC32, crc, sizeof(crc)); > + } > } > + if (size > skip) Same as previous: just skipping the work when the buffer is not big enough seems broken. > avio_write(pb, buf + skip, size - skip); > > if (keep_buffer) { Regards,
lance.lmwang@gmail.com: > From: Limin Wang <lance.lmwang@gmail.com> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > --- > libavformat/matroskaenc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c > index 784973a951..f0474da44f 100644 > --- a/libavformat/matroskaenc.c > +++ b/libavformat/matroskaenc.c > @@ -374,9 +374,12 @@ static void end_ebml_master_crc32(AVIOContext *pb, AVIOContext **dyn_cp, > put_ebml_length(pb, size, length_size); > if (mkv->write_crc) { > skip = 6; /* Skip reserved 6-byte long void element from the dynamic buffer. */ > + if (size > skip) { > AV_WL32(crc, av_crc(av_crc_get_table(AV_CRC_32_IEEE_LE), UINT32_MAX, buf + skip, size - skip) ^ UINT32_MAX); > put_ebml_binary(pb, EBML_ID_CRC32, crc, sizeof(crc)); > + } > } > + if (size > skip) > avio_write(pb, buf + skip, size - skip); > > if (keep_buffer) { > I sent a patch containing proper checks for this and other allocations in this muxer here [1]. - Andreas PS: avio_close_dyn_buf() is even worse: Besides the design flaw of freeing a resource without setting the pointer to it to NULL, it returns a size of -AV_INPUT_BUFFER_PADDING_SIZE if a memory allocation failure happened (but not if the arbitrary limit of INT_MAX/2 has been surpassed); and this despite its documentation not allowing returning negative values at all. (And it returns the current position of the write pointer as size and zeroes what comes immediately after, even if a seek to the front has happened.) [1]: http://ffmpeg.org/pipermail/ffmpeg-devel/2020-April/261704.html
Andreas Rheinhardt (12020-04-30): > I sent a patch containing proper checks for this and other allocations > in this muxer here [1]. Thanks. > PS: avio_close_dyn_buf() is even worse: Besides the design flaw of > freeing a resource without setting the pointer to it to NULL, it returns > a size of -AV_INPUT_BUFFER_PADDING_SIZE if a memory allocation failure > happened (but not if the arbitrary limit of INT_MAX/2 has been > surpassed); and this despite its documentation not allowing returning > negative values at all. (And it returns the current position of the > write pointer as size and zeroes what comes immediately after, even if a > seek to the front has happened.) The av_dynbuf_write() API I proposed some time ago allows proper error check at the end (and I even intend to make it a little more unavoidable in the next iteration). And as a bonus, it uses an on-stack buffer as long as it fits (it is based on BPrint). I have a small intro written about it if people are interested. Regards,
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index 784973a951..f0474da44f 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -374,9 +374,12 @@ static void end_ebml_master_crc32(AVIOContext *pb, AVIOContext **dyn_cp, put_ebml_length(pb, size, length_size); if (mkv->write_crc) { skip = 6; /* Skip reserved 6-byte long void element from the dynamic buffer. */ + if (size > skip) { AV_WL32(crc, av_crc(av_crc_get_table(AV_CRC_32_IEEE_LE), UINT32_MAX, buf + skip, size - skip) ^ UINT32_MAX); put_ebml_binary(pb, EBML_ID_CRC32, crc, sizeof(crc)); + } } + if (size > skip) avio_write(pb, buf + skip, size - skip); if (keep_buffer) {