diff mbox series

[FFmpeg-devel,2/5] avformat/matroskaenc: fix invalid pointer access if avio_get_dyn_buf failed

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
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Limin Wang April 29, 2020, 3:14 p.m. UTC
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(+)

Comments

Nicolas George April 29, 2020, 3:19 p.m. UTC | #1
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,
Andreas Rheinhardt April 29, 2020, 10:33 p.m. UTC | #2
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
Nicolas George April 29, 2020, 10:37 p.m. UTC | #3
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 mbox series

Patch

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) {