[FFmpeg-devel] [PATCH 1/8] png: Clear up the calculation of max packet size

[Michael Niedermayer]

On Sun, Mar 29, 2015 at 11:05:40AM +0000, Donny Yang wrote:
> Signed-off-by: Donny Yang <work at kota.moe>
> ---
>  libavcodec/pngenc.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c
> index d6233d0..3697dbb 100644
> --- a/libavcodec/pngenc.c
> +++ b/libavcodec/pngenc.c
> @@ -360,12 +360,23 @@ static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,
>          return -1;
>  
>      enc_row_size    = deflateBound(&s->zstream, row_size);
> -    max_packet_size = avctx->height * (int64_t)(enc_row_size +
> -                                       ((enc_row_size + IOBUF_SIZE - 1) / IOBUF_SIZE) * 12)
> -                      + FF_MIN_BUFFER_SIZE;
> +    max_packet_size =
> +        8 +             // PNGSIG
> +        13 + 12 +       // IHDR
> +        9 + 12 +        // pHYs
> +        1 + 12 +        // sRGB
> +        32 + 12 +       // cHRM
> +        4 + 12 +        // gAMA
> +        256 * 3 + 12 +  // PLTE
> +        256 + 12 +      // tRNS
> +        avctx->height * (
> +            enc_row_size +
> +            12 * (((int64_t)enc_row_size + IOBUF_SIZE - 1) / IOBUF_SIZE) // 12 * ceil(enc_row_size / IOBUF_SIZE)
> +        );
>      if (max_packet_size > INT_MAX)
>          return AVERROR(ENOMEM);
> -    if ((ret = ff_alloc_packet2(avctx, pkt, max_packet_size)) < 0)
> +    ret = ff_alloc_packet2(avctx, pkt, max_packet_size < FF_MIN_BUFFER_SIZE ? FF_MIN_BUFFER_SIZE : max_packet_size);

i understand the change is well meant and it makes the allocated size
closer to what is actually written but
this would require the list of sizes to be kept in sync with what is
actually written. This is a additional maintaince burden and has
serious consequence if one makes a change and then forgets updating
the size as it could result in out of array writes becoming possible
and then theres a possibly exploitable security issues if that happens

so i suggest to leave the "+ FF_MIN_BUFFER_SIZE" in there unless there
is something else that protects against more data being written than
what is listed here


> +    if (ret)

(ret < 0)

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150329/f27a8e28/attachment.asc>