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

[Andreas Rheinhardt]

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 at 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