[FFmpeg-devel,1/3] avformat/wavenc: Fix leak and segfault on reallocation error

Up until now, the wav muxer used a reallocation of the form ptr =
av_realloc(ptr, size); that leaks upon error. Furthermore, if a
failed reallocation happened when writing the trailer, a segfault
would occur due to avio_write(NULL, size) because the muxer only
prints an error message upon allocation error, but does not return
the error.

Moreover setting the pointer to the buffer to NULL on error seems to
be done on purpose in order to record that an error has occured so that
outputting the peak values is no longer attempted. This behaviour has
been retained by simply disabling whether peak data should be written
if an error occurs.

Finally, the reallocation is now done once per peak block and not once
per peak block per channel; it is also done with av_fast_realloc and not
with a linear size increase.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/wavenc.c | 53 ++++++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 24 deletions(-)

Andreas Rheinhardt:
> Up until now, the wav muxer used a reallocation of the form ptr =
> av_realloc(ptr, size); that leaks upon error. Furthermore, if a
> failed reallocation happened when writing the trailer, a segfault
> would occur due to avio_write(NULL, size) because the muxer only
> prints an error message upon allocation error, but does not return
> the error.
> 
> Moreover setting the pointer to the buffer to NULL on error seems to
> be done on purpose in order to record that an error has occured so that
> outputting the peak values is no longer attempted. This behaviour has
> been retained by simply disabling whether peak data should be written
> if an error occurs.
> 
> Finally, the reallocation is now done once per peak block and not once
> per peak block per channel; it is also done with av_fast_realloc and not
> with a linear size increase.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/wavenc.c | 53 ++++++++++++++++++++++++--------------------
>  1 file changed, 29 insertions(+), 24 deletions(-)
> 
> diff --git a/libavformat/wavenc.c b/libavformat/wavenc.c
> index 1027f107ee..9ef72f6e1c 100644
> --- a/libavformat/wavenc.c
> +++ b/libavformat/wavenc.c
> @@ -50,8 +50,6 @@
>  #define RF64_NEVER  0
>  #define RF64_ALWAYS 1
>  
> -#define PEAK_BUFFER_SIZE   1024
> -
>  typedef enum {
>      PEAK_OFF = 0,
>      PEAK_ON,
> @@ -72,8 +70,9 @@ typedef struct WAVMuxContext {
>      int64_t maxpts;
>      int16_t *peak_maxpos, *peak_maxneg;
>      uint32_t peak_num_frames;
> -    uint32_t peak_outbuf_size;
> +    unsigned peak_outbuf_size;
>      uint32_t peak_outbuf_bytes;
> +    unsigned size_increment;
>      uint8_t *peak_output;
>      int last_duration;
>      int write_bext;
> @@ -172,15 +171,15 @@ static av_cold int peak_init_writer(AVFormatContext *s)
>                 "Writing 16 bit peak for 8 bit audio does not make sense\n");
>          return AVERROR(EINVAL);
>      }
> +    if (par->channels > INT_MAX / (wav->peak_bps * wav->peak_ppv))
> +        return AVERROR(ERANGE);
> +    wav->size_increment = par->channels * wav->peak_bps * wav->peak_ppv;
>  
>      wav->peak_maxpos = av_mallocz_array(par->channels, sizeof(*wav->peak_maxpos));
>      wav->peak_maxneg = av_mallocz_array(par->channels, sizeof(*wav->peak_maxneg));
> -    wav->peak_output = av_malloc(PEAK_BUFFER_SIZE);
> -    if (!wav->peak_maxpos || !wav->peak_maxneg || !wav->peak_output)
> +    if (!wav->peak_maxpos || !wav->peak_maxneg)
>          goto nomem;
>  
> -    wav->peak_outbuf_size = PEAK_BUFFER_SIZE;
> -
>      return 0;
>  
>  nomem:
> @@ -188,14 +187,24 @@ nomem:
>      return AVERROR(ENOMEM);
>  }
>  
> -static void peak_write_frame(AVFormatContext *s)
> +static int peak_write_frame(AVFormatContext *s)
>  {
>      WAVMuxContext *wav = s->priv_data;
>      AVCodecParameters *par = s->streams[0]->codecpar;
> +    unsigned new_size = wav->peak_outbuf_bytes + wav->size_increment;
> +    uint8_t *tmp;
>      int c;
>  
> -    if (!wav->peak_output)
> -        return;
> +    if (new_size > INT_MAX) {
> +        wav->write_peak = PEAK_OFF;
> +        return AVERROR(ERANGE);
> +    }
> +    tmp = av_fast_realloc(wav->peak_output, &wav->peak_outbuf_size, new_size);
> +    if (!tmp) {
> +        wav->write_peak = PEAK_OFF;
> +        return AVERROR(ENOMEM);
> +    }
> +    wav->peak_output = tmp;
>  
>      for (c = 0; c < par->channels; c++) {
>          wav->peak_maxneg[c] = -wav->peak_maxneg[c];
> @@ -209,17 +218,6 @@ static void peak_write_frame(AVFormatContext *s)
>              wav->peak_maxpos[c] =
>                  FFMAX(wav->peak_maxpos[c], wav->peak_maxneg[c]);
>  
> -        if (wav->peak_outbuf_size - wav->peak_outbuf_bytes <
> -            wav->peak_format * wav->peak_ppv) {
> -            wav->peak_outbuf_size += PEAK_BUFFER_SIZE;
> -            wav->peak_output = av_realloc(wav->peak_output,
> -                                          wav->peak_outbuf_size);
> -            if (!wav->peak_output) {
> -                av_log(s, AV_LOG_ERROR, "No memory for peak data\n");
> -                return;
> -            }
> -        }
> -
>          if (wav->peak_format == PEAK_FORMAT_UINT8) {
>              wav->peak_output[wav->peak_outbuf_bytes++] =
>                  wav->peak_maxpos[c];
> @@ -241,6 +239,8 @@ static void peak_write_frame(AVFormatContext *s)
>          wav->peak_maxneg[c] = 0;
>      }
>      wav->peak_num_frames++;
> +
> +    return 0;
>  }
>  
>  static int peak_write_chunk(AVFormatContext *s)
> @@ -254,8 +254,11 @@ static int peak_write_chunk(AVFormatContext *s)
>      char timestamp[28];
>  
>      /* Peak frame of incomplete block at end */
> -    if (wav->peak_block_pos)
> -        peak_write_frame(s);
> +    if (wav->peak_block_pos) {
> +        int ret = peak_write_frame(s);
> +        if (ret < 0)
> +            return ret;
> +    }
>  
>      memset(timestamp, 0, sizeof(timestamp));
>      if (!(s->flags & AVFMT_FLAG_BITEXACT)) {
> @@ -386,7 +389,9 @@ static int wav_write_packet(AVFormatContext *s, AVPacket *pkt)
>              if (++c == s->streams[0]->codecpar->channels) {
>                  c = 0;
>                  if (++wav->peak_block_pos == wav->peak_block_size) {
> -                    peak_write_frame(s);
> +                    int ret = peak_write_frame(s);
> +                    if (ret < 0)
> +                        return ret;
>                      wav->peak_block_pos = 0;
>                  }
>              }
> 
Will apply this patchset tomorrow unless there are objections.

- Andreas