[FFmpeg-devel] [PATCH] ffmpeg: check fclose return values

[Ganesh Ajjanagadde]

On Wed, Jan 6, 2016 at 9:00 PM, Ganesh Ajjanagadde
<gajjanagadde at gmail.com> wrote:
> In the spirit of commit a956840cbc. Simple method to reproduce:
> pass -vstats_file /dev/full to ffmpeg.
>
> All raw fclose usages in ffmpeg.c taken care of here.
>
> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
> ---
>  ffmpeg.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/ffmpeg.c b/ffmpeg.c
> index 0c83165..858e6d7 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -554,8 +554,12 @@ static void ffmpeg_cleanup(int ret)
>          av_freep(&input_streams[i]);
>      }
>
> -    if (vstats_file)
> -        fclose(vstats_file);
> +    if (vstats_file) {
> +        if ((ret = fclose(vstats_file)) < 0)
> +            av_log(NULL, AV_LOG_ERROR,
> +                   "Error closing vstats file, loss of information possible: %s\n",
> +                   av_err2str(ret));
> +    }
>      av_freep(&vstats_filename);
>
>      av_freep(&input_streams);
> @@ -4200,7 +4204,10 @@ static int transcode(void)
>              ost = output_streams[i];
>              if (ost) {
>                  if (ost->logfile) {
> -                    fclose(ost->logfile);
> +                    if ((ret = fclose(ost->logfile)) < 0)
> +                        av_log(NULL, AV_LOG_ERROR,
> +                               "Error closing logfile, loss of information possible: %s\n",
> +                               av_err2str(ret));
>                      ost->logfile = NULL;
>                  }
>                  av_freep(&ost->forced_kf_pts);
> --
> 2.6.4
>

Should have mentioned this earlier, but this fclose related business
was due to listening to:
https://www.irill.org/events/ghm-gnu-hackers-meeting/videos/jim-meyering-goodbye-world-the-perils-of-relying-on-output-streams-in-c,
slides: https://www.gnu.org/ghm/2011/paris/slides/jim-meyering-goodbye-world.pdf.

For a more sustainable solution, we should create an API in e.g
libavutil for this IMHO, since fclose is called not just in ffmpeg but
also in libavfilter. There may be 2 ways of doing this:
1. Developing it organically in FFmpeg.
2. Using gnulib and associated close_stream/other API's (e.g
https://github.com/coreutils/gnulib/blob/master/lib/close-stream.c) as
suggested in the talk as a basis. There are some problems:
a) Licensing - it is GPL. There is some mention regarding relicensing
for gnulib, https://www.gnu.org/software/gnulib/manual/html_node/Copyright.html,
so it may be ok to relicense under lgpl. However, I am not a lawyer.
b) Care may need to be taken to make sure it is portable enough, and
refactoring may be necessary.