[FFmpeg-devel] [PATCH 04/10] ffserver.c: Check allocations

[Michael Niedermayer]

On Mon, May 28, 2018 at 08:27:05PM +0200, Stephan Holljes wrote:
> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
> ---
>  ffserver.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 81 insertions(+), 6 deletions(-)
> 
> diff --git a/ffserver.c b/ffserver.c
> index 66bf7ac..171da51 100644
> --- a/ffserver.c
> +++ b/ffserver.c
> @@ -227,12 +227,25 @@ void write_segment(struct Client *c)
>          }
>          

>          avio_buffer = (unsigned char*) av_malloc(AV_BUFSIZE);

unrelated but you dont need the cast


> +        if (!avio_buffer) {
> +            av_log(NULL, AV_LOG_ERROR, "Could not allocate avio_buffer\n");

some context instead of NULL would be nice so errors arent just all "global"




> +            avformat_free_context(fmt_ctx);
> +            client_disconnect(c, 0);
> +            return;
> +        }
>          avio_ctx = avio_alloc_context(avio_buffer, AV_BUFSIZE, 0, &info, &segment_read, NULL, NULL);
> -        
> +        if (!avio_ctx) {
> +            av_log(NULL, AV_LOG_ERROR, "Could not allocate avio_ctx\n");
> +            avformat_free_context(fmt_ctx);
> +            av_free(avio_buffer);
> +            client_disconnect(c, 0);
> +            return;
> +        }
>          fmt_ctx->pb = avio_ctx;
>          ret = avformat_open_input(&fmt_ctx, NULL, seg->ifmt, NULL);
>          if (ret < 0) {
>              av_log(avio_ctx, AV_LOG_ERROR, "Could not open input\n");
> +            avformat_close_input(&fmt_ctx);
>              av_free(avio_ctx->buffer);
>              avio_context_free(&avio_ctx);
>              client_disconnect(c, 0);
> @@ -242,6 +255,7 @@ void write_segment(struct Client *c)
>          ret = avformat_find_stream_info(fmt_ctx, NULL);
>          if (ret < 0) {
>              av_log(fmt_ctx, AV_LOG_ERROR, "Could not find stream information\n");
> +            avformat_close_input(&fmt_ctx);
>              av_free(avio_ctx->buffer);
>              avio_context_free(&avio_ctx);
>              client_disconnect(c, 0);
> @@ -274,9 +288,8 @@ void write_segment(struct Client *c)
>                  return;
>              }
>          }
> -        avformat_close_input(&fmt_ctx);
>          av_free(avio_ctx->buffer);
> -        avformat_free_context(fmt_ctx);
> +        avformat_close_input(&fmt_ctx);
>          avio_context_free(&avio_ctx);
>          pthread_mutex_lock(&c->buffer_lock);
>          av_fifo_drain(c->buffer, sizeof(struct Segment*));
> @@ -369,13 +382,25 @@ void *accept_thread(void *arg)
>          }
>          
>          avio_buffer = av_malloc(AV_BUFSIZE);
> +        if (!avio_buffer) {
> +            av_log(NULL, AV_LOG_ERROR, "Could not allocate output format context.\n");
> +            publisher_cancel_reserve(pub);
> +            info->httpd->close(server, client);
> +            continue;
> +        }

>          ffinfo = av_malloc(sizeof(struct FFServerInfo));

not about this patch but existing code but
using a code like sizeof(*ffinfo) would avoid having to duplicate the type
in the source code (one less opertunity to make mistakes)

[...]

> @@ -521,9 +555,39 @@ void *run_server(void *arg) {
>      ainfo.config = config;
>      
>      rinfos = av_mallocz_array(config->nb_streams, sizeof(struct ReadInfo));
> +    if (!rinfos) {
> +        av_log(NULL, AV_LOG_ERROR, "Could not allocate read infos.\n");
> +        av_free(pubs);
> +        av_free(ifmt_ctxs);
> +        return NULL;
> +    }
>      winfos_p = av_mallocz_array(config->nb_streams, sizeof(struct WriteInfo*));
> +    if (!winfos_p) {
> +        av_log(NULL, AV_LOG_ERROR, "Could not allocate write info pointers.\n");
> +        av_free(pubs);
> +        av_free(ifmt_ctxs);
> +        av_free(rinfos);
> +        return NULL;
> +    }
>      r_threads = av_mallocz_array(config->nb_streams, sizeof(pthread_t));
> +    if (!r_threads) {
> +        av_log(NULL, AV_LOG_ERROR, "Could not allocate read thread handles.\n");
> +        av_free(pubs);
> +        av_free(ifmt_ctxs);
> +        av_free(rinfos);
> +        av_free(winfos_p);
> +        return NULL;
> +    }
>      w_threads_p = av_mallocz_array(config->nb_streams, sizeof(pthread_t*));
> +    if (!w_threads_p) {
> +        av_log(NULL, AV_LOG_ERROR, "Could not allocate write thread handle pointers.\n");
> +        av_free(pubs);
> +        av_free(ifmt_ctxs);
> +        av_free(rinfos);
> +        av_free(winfos_p);
> +        av_free(r_threads);
> +        return NULL;
> +    }

some goto cleanup_error
and a cleanup_error that deallocates and returns
would avoid having to duplicate the cleanup code


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180528/23dac129/attachment.sig>